Re: [PATCH] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

Re: [PATCH] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

On Fri, 2026-04-24 at 00:30 +0530, karthikeyan K S wrote:
> From c50ff07baf2032ca12133775c61c50a38e8a2029 Mon Sep 17 00:00:00 2001
> From: Karthikeyan KS <karthiproffesional@gmail.com>
> Date: Thu, 23 Apr 2026 21:26:08 +0300
> Subject: [PATCH] soc: aspeed: lpc-snoop: Fix usercopy overflow in
>  snoop_file_read
> 
> snoop_file_read() passes the userspace count directly to
> kfifo_to_user() without clamping. The kfifo backing buffer is
> 2048 bytes (SNOOP_FIFO_SIZE), allocated from kmalloc-2k slab.
> A read larger than 2048 bytes triggers a BUG under
> CONFIG_HARDENED_USERCOPY:
> 
>   kernel BUG at mm/usercopy.c:99!
> 
> Reproducer:
>   hexdump /dev/aspeed-lpc-snoop0

Can you provide more details on how you achieved this result?
__kfifo_to_user() clamps the provided value to the content of the fifo
and has done in its current form since 2e956fb32056 ("kfifo: replace
the old non generic API").

> 
> Fix by clamping count to SNOOP_FIFO_SIZE before the copy.
> 
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc
> chardev")
> Cc: stable@vger.kernel.org
> Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index b03310c0830d..5b59e826cc68 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -125,6 +125,7 @@ static ssize_t snoop_file_read(struct file *file, char
> __user *buffer,
>         if (ret == -ERESTARTSYS)
>             return -EINTR;
>     }
> +   count = min(count, (size_t)SNOOP_FIFO_SIZE);
>     ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
>     if (ret)
>         return ret;

[PATCH v2] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

put_fifo_with_discard() violates kfifo's single-producer/single-consumer
lock-free contract by calling both kfifo_skip() (consumer op: out++) and
kfifo_put() (producer op: in++) from the IRQ handler context.
kfifo_skip() increments 'out' without a memory barrier, while
kfifo_put() increments 'in' with smp_wmb(). On ARM (weakly ordered), a
concurrent reader on another CPU can observe the new 'in' but a stale
'out', causing (in - out) to exceed the buffer size.

__kfifo_to_user() clamps the copy length to (in - out), but since that
value is already corrupted by the race, the clamp is ineffective. The
subsequent kfifo_copy_to_user() ring-buffer split produces a second
chunk exceeding the 2048-byte kmalloc-2k slab object, triggering:

  Backtrace:
[    2.972611] usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-2k' (offset 0, size 2049)!
[    2.974191] ------------[ cut here ]------------
[    2.974677] kernel BUG at mm/usercopy.c:99!
[    2.975068] Internal error: Oops - BUG: 0 [#1] SMP ARM
[    2.975755] Modules linked in: post_injector(O)
[    2.976668] CPU: 0 PID: 1 Comm: init Tainted: G           O      5.15.178 #4
[    2.977316] Hardware name: Generic DT based system
[    2.977848] PC is at usercopy_abort+0x80/0xa8
[    2.978931] LR is at vprintk_emit+0xf0/0x230
[    2.979296] pc : [<8095c3a0>]    lr : [<8017cb2c>]    psr: 60000153
[    2.979781] sp : 810e3dc8  ip : 810e3d38  fp : 810e3dec
[    2.980192] r10: 00000003  r9 : 811ea801  r8 : 811ea800
[    2.980616] r7 : 00000001  r6 : 00000801  r5 : 00000801  r4 : 00000000
[    2.981104] r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : 00000066
[    2.981712] Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
[    2.982363] Control: 00c5387d  Table: 81a64008  DAC: 00000051
[    2.982881] Register r0 information: non-paged memory
[    2.983662] Register r1 information: NULL pointer
[    2.984074] Register r2 information: NULL pointer
[    2.984474] Register r3 information: NULL pointer
[    2.984879] Register r4 information: NULL pointer
[    2.985279] Register r5 information: non-paged memory
[    2.985704] Register r6 information: non-paged memory
[    2.986118] Register r7 information: non-paged memory
[    2.986553] Register r8 information: slab kmalloc-2k start 811ea800 pointer offset 0 size 2048
[    2.987789] Register r9 information: slab kmalloc-2k start 811ea800 pointer offset 1 size 2048
[    2.988541] Register r10 information: non-paged memory
[    2.988971] Register r11 information: non-slab/vmalloc memory
[    2.989511] Register r12 information: non-slab/vmalloc memory
[    2.990064] Process init (pid: 1, stack limit = 0x041252af)
[    2.990631] Stack: (0x810e3dc8 to 0x810e4000)
[    2.991119] 3dc0:                   80c2ceb4 80c29fd4 80c1f6d4 00000000 00000801 811ea801
[    2.992044] 3de0: 810e3e1c 810e3df0 802e3a94 8095c32c 00000801 801168b0 811ea800 811ea800
[    2.992707] 3e00: 00000801 00000001 811eb001 00000001 810e3e44 810e3e20 802e8774 802e3978
[    2.993336] 3e20: 810e3e44 810e3e30 00000801 00001000 7edc2593 811ea800 810e3e6c 810e3e48
[    2.993971] 3e40: 804dedf0 802e8604 811d584c 00001000 810e3e98 7edc1d94 00001000 810e2000
[    2.994734] 3e60: 810e3e94 810e3e70 804def50 804ded54 810e3e98 804d3f28 00000000 811d586c
[    2.995377] 3e80: 810e3e98 00000001 810e3ed4 810e3e98 80579a58 804def0c 810e3f34 810e3ea8
[    2.996024] 3ea0: 80282c74 80282744 00000000 37f2b80b ffffe000 819a2cc0 810e3f68 00000001
[    2.996684] 3ec0: 805799a8 00001000 810e3f64 810e3ed8 802ee040 805799b4 00000003 1cd5b000
[    2.997310] 3ee0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.997987] 3f00: 801002c4 37f2b80b 810e2000 810e2000 810e3fb0 00000000 801002c4 80314ce0
[    2.998678] 3f20: 00000000 00000003 014fc580 00001000 7edc1d94 37f2b80b 810e2000 819a2cc0
[    2.999268] 3f40: 819a2cc0 00000000 00000000 801002c4 810e2000 00000003 810e3f94 810e3f68
[    3.000024] 3f60: 802ee740 802edf90 00000000 00000000 00000800 37f2b80b 00000003 014fc580
[    3.000688] 3f80: 00006f2c 00000003 810e3fa4 810e3f98 802ee7e4 802ee6dc 00000000 810e3fa8
[    3.001364] 3fa0: 80100080 802ee7d8 00000003 014fc580 00000003 7edc1d94 00001000 00000000
[    3.002389] 3fc0: 00000003 014fc580 00006f2c 00000003 7edc2f10 00000000 00000001 00000000
[    3.003042] 3fe0: 000000ac 7edc1d18 0002c174 0002c190 60000150 00000003 00000000 00000000
[    3.003773] Backtrace:
[    3.004236] [<8095c320>] (usercopy_abort) from [<802e3a94>] (__check_heap_object+0x128/0x150)
[    3.005187] [<802e396c>] (__check_heap_object) from [<802e8774>] (__check_object_size+0x17c/0x1e0)
[    3.005996]  r8:00000001 r7:811eb001 r6:00000001 r5:00000801 r4:811ea800
[    3.006555] [<802e85f8>] (__check_object_size) from [<804dedf0>] (kfifo_copy_to_user+0xa8/0x1b8)
[    3.007290]  r7:811ea800 r6:7edc2593 r5:00001000 r4:00000801
[    3.007766] [<804ded48>] (kfifo_copy_to_user) from [<804def50>] (__kfifo_to_user+0x50/0x70)
[    3.008418]  r9:810e2000 r8:00001000 r7:7edc1d94 r6:810e3e98 r5:00001000 r4:811d584c
[    3.009031] [<804def00>] (__kfifo_to_user) from [<80579a58>] (snoop_file_read+0xb0/0xf8)
[    3.009683]  r6:00000001 r5:810e3e98 r4:811d586c
[    3.010055] [<805799a8>] (snoop_file_read) from [<802ee040>] (vfs_read+0xbc/0x328)
[    3.010686]  r8:00001000 r7:805799a8 r6:00000001 r5:810e3f68 r4:819a2cc0
[    3.011182] [<802edf84>] (vfs_read) from [<802ee740>] (ksys_read+0x70/0xfc)
[    3.011762]  r10:00000003 r9:810e2000 r8:801002c4 r7:00000000 r6:00000000 r5:819a2cc0
[    3.012451]  r4:819a2cc0
[    3.012677] [<802ee6d0>] (ksys_read) from [<802ee7e4>] (sys_read+0x18/0x1c)
[    3.013261]  r7:00000003 r6:00006f2c r5:014fc580 r4:00000003
[    3.013724] [<802ee7cc>] (sys_read) from [<80100080>] (ret_fast_syscall+0x0/0x48)
[    3.014330] Exception stack(0x810e3fa8 to 0x810e3ff0)
[    3.014767] 3fa0:                   00000003 014fc580 00000003 7edc1d94 00001000 00000000
[    3.015379] 3fc0: 00000003 014fc580 00006f2c 00000003 7edc2f10 00000000 00000001 00000000
[    3.016030] 3fe0: 000000ac 7edc1d18 0002c174 0002c190
[    3.016641] Code: e1cd40fc e58dc000 e59f0024 ebfff741 (e7f001f2)
[    3.017733] ---[ end trace d9f7e472f48076c9 ]---
[    3.018380] Kernel panic - not syncing: Fatal exception
[    3.019190] ---[ end Kernel panic - not syncing: Fatal exception ]---


This is reproducible on AST2500 BMC (dual-core ARM Cortex-A7) when
BIOS floods POST codes while userspace concurrently reads
/dev/aspeed-lpc-snoop0.

Fix by:

1. Clamping 'count' to SNOOP_FIFO_SIZE in snoop_file_read() so that
   copy_to_user() can never exceed the slab object boundary regardless
   of kfifo pointer state.

2. Protecting the kfifo_skip() + kfifo_put() sequence in
   put_fifo_with_discard() and the kfifo_to_user() call in
   snoop_file_read() with a spinlock, ensuring pointer updates are
   atomic with respect to concurrent access and restoring the
   single-writer invariant for each pointer.

   put_fifo_with_discard() is only called from hardirq context where
   interrupts are already disabled, so plain spin_lock/spin_unlock is
   used. snoop_file_read() runs in process context and must use
   spin_lock_irqsave to prevent deadlock with the IRQ handler.

Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
Andrew,

You're right — __kfifo_to_user() clamps to (in - out). The issue is
that (in - out) itself is corrupted by a race in put_fifo_with_discard().

The function calls kfifo_skip() (out++) then kfifo_put() (in++) without
synchronization. On SMP ARM, the reader observes fresh 'in' (barrier in
kfifo_put) but stale 'out' (no barrier in kfifo_skip), making (in - out)
exceed the buffer size. The clamp then passes through the corrupted value.

Reproduced on QEMU ast2500-evb by injecting POST codes and simulating
the race outcome. Also observed intermittently on physical dual-core
AST2600 under heavy POST code traffic.

Changes since v1:
- Root cause identified as SMP race, not missing read-side clamp
- Added spinlock to put_fifo_with_discard() and snoop_file_read()
- spin_lock() in IRQ context, spin_lock_irqsave() in process context
- Retained count clamp as defense in depth

 drivers/soc/aspeed/aspeed-lpc-snoop.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index eceeaf8..d084492 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -60,6 +60,7 @@ struct aspeed_lpc_snoop_model_data {
 
 struct aspeed_lpc_snoop_channel {
 	struct kfifo		fifo;
+	spinlock_t		lock;
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
 };
@@ -83,8 +84,11 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 {
 	struct aspeed_lpc_snoop_channel *chan = snoop_file_to_chan(file);
 	unsigned int copied;
+	unsigned long flags;
 	int ret = 0;
 
+	count = min_t(size_t, count, SNOOP_FIFO_SIZE);
+
 	if (kfifo_is_empty(&chan->fifo)) {
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
@@ -93,7 +97,11 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 		if (ret == -ERESTARTSYS)
 			return -EINTR;
 	}
+
+	spin_lock_irqsave(&chan->lock, flags);
 	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
+	spin_unlock_irqrestore(&chan->lock, flags);
+
 	if (ret)
 		return ret;
 
@@ -121,9 +129,11 @@ static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
 {
 	if (!kfifo_initialized(&chan->fifo))
 		return;
+	spin_lock(&chan->lock);
 	if (kfifo_is_full(&chan->fifo))
 		kfifo_skip(&chan->fifo);
 	kfifo_put(&chan->fifo, val);
+	spin_unlock(&chan->lock);
 	wake_up_interruptible(&chan->wq);
 }
 
@@ -192,6 +202,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		of_device_get_match_data(dev);
 
 	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
+	spin_lock_init(&lpc_snoop->chan[channel].lock);
 	/* Create FIFO datastructure */
 	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
 			 SNOOP_FIFO_SIZE, GFP_KERNEL);

Re: [PATCH v2] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

Hi Karthikeyan,

On Sat, 2026-05-23 at 17:35 +0000, Karthikeyan KS wrote:
> put_fifo_with_discard() violates kfifo's single-producer/single-consumer
> lock-free contract by calling both kfifo_skip() (consumer op: out++) and
> kfifo_put() (producer op: in++) from the IRQ handler context.
> kfifo_skip() increments 'out' without a memory barrier, while
> kfifo_put() increments 'in' with smp_wmb(). On ARM (weakly ordered), a
> concurrent reader on another CPU can observe the new 'in' but a stale
> 'out', causing (in - out) to exceed the buffer size.
> 
> __kfifo_to_user() clamps the copy length to (in - out), but since that
> value is already corrupted by the race, the clamp is ineffective. The
> subsequent kfifo_copy_to_user() ring-buffer split produces a second
> chunk exceeding the 2048-byte kmalloc-2k slab object, triggering:
> 
>   Backtrace:
> [    2.972611] usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-2k' (offset 0, size 2049)!
> [    2.974191] ------------[ cut here ]------------
> [    2.974677] kernel BUG at mm/usercopy.c:99!
> [    2.975068] Internal error: Oops - BUG: 0 [#1] SMP ARM
> [    2.975755] Modules linked in: post_injector(O)
> [    2.976668] CPU: 0 PID: 1 Comm: init Tainted: G           O      5.15.178 #4
> [    2.977316] Hardware name: Generic DT based system
> [    2.977848] PC is at usercopy_abort+0x80/0xa8
> [    2.978931] LR is at vprintk_emit+0xf0/0x230
> [    2.979296] pc : [<8095c3a0>]    lr : [<8017cb2c>]    psr: 60000153
> [    2.979781] sp : 810e3dc8  ip : 810e3d38  fp : 810e3dec
> [    2.980192] r10: 00000003  r9 : 811ea801  r8 : 811ea800
> [    2.980616] r7 : 00000001  r6 : 00000801  r5 : 00000801  r4 : 00000000
> [    2.981104] r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : 00000066
> [    2.981712] Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> [    2.982363] Control: 00c5387d  Table: 81a64008  DAC: 00000051
> [    2.982881] Register r0 information: non-paged memory
> [    2.983662] Register r1 information: NULL pointer
> [    2.984074] Register r2 information: NULL pointer
> [    2.984474] Register r3 information: NULL pointer
> [    2.984879] Register r4 information: NULL pointer
> [    2.985279] Register r5 information: non-paged memory
> [    2.985704] Register r6 information: non-paged memory
> [    2.986118] Register r7 information: non-paged memory
> [    2.986553] Register r8 information: slab kmalloc-2k start 811ea800 pointer offset 0 size 2048
> [    2.987789] Register r9 information: slab kmalloc-2k start 811ea800 pointer offset 1 size 2048
> [    2.988541] Register r10 information: non-paged memory
> [    2.988971] Register r11 information: non-slab/vmalloc memory
> [    2.989511] Register r12 information: non-slab/vmalloc memory
> [    2.990064] Process init (pid: 1, stack limit = 0x041252af)
> [    2.990631] Stack: (0x810e3dc8 to 0x810e4000)
> [    2.991119] 3dc0:                   80c2ceb4 80c29fd4 80c1f6d4 00000000 00000801 811ea801
> [    2.992044] 3de0: 810e3e1c 810e3df0 802e3a94 8095c32c 00000801 801168b0 811ea800 811ea800
> [    2.992707] 3e00: 00000801 00000001 811eb001 00000001 810e3e44 810e3e20 802e8774 802e3978
> [    2.993336] 3e20: 810e3e44 810e3e30 00000801 00001000 7edc2593 811ea800 810e3e6c 810e3e48
> [    2.993971] 3e40: 804dedf0 802e8604 811d584c 00001000 810e3e98 7edc1d94 00001000 810e2000
> [    2.994734] 3e60: 810e3e94 810e3e70 804def50 804ded54 810e3e98 804d3f28 00000000 811d586c
> [    2.995377] 3e80: 810e3e98 00000001 810e3ed4 810e3e98 80579a58 804def0c 810e3f34 810e3ea8
> [    2.996024] 3ea0: 80282c74 80282744 00000000 37f2b80b ffffe000 819a2cc0 810e3f68 00000001
> [    2.996684] 3ec0: 805799a8 00001000 810e3f64 810e3ed8 802ee040 805799b4 00000003 1cd5b000
> [    2.997310] 3ee0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    2.997987] 3f00: 801002c4 37f2b80b 810e2000 810e2000 810e3fb0 00000000 801002c4 80314ce0
> [    2.998678] 3f20: 00000000 00000003 014fc580 00001000 7edc1d94 37f2b80b 810e2000 819a2cc0
> [    2.999268] 3f40: 819a2cc0 00000000 00000000 801002c4 810e2000 00000003 810e3f94 810e3f68
> [    3.000024] 3f60: 802ee740 802edf90 00000000 00000000 00000800 37f2b80b 00000003 014fc580
> [    3.000688] 3f80: 00006f2c 00000003 810e3fa4 810e3f98 802ee7e4 802ee6dc 00000000 810e3fa8
> [    3.001364] 3fa0: 80100080 802ee7d8 00000003 014fc580 00000003 7edc1d94 00001000 00000000
> [    3.002389] 3fc0: 00000003 014fc580 00006f2c 00000003 7edc2f10 00000000 00000001 00000000
> [    3.003042] 3fe0: 000000ac 7edc1d18 0002c174 0002c190 60000150 00000003 00000000 00000000
> [    3.003773] Backtrace:
> [    3.004236] [<8095c320>] (usercopy_abort) from [<802e3a94>] (__check_heap_object+0x128/0x150)
> [    3.005187] [<802e396c>] (__check_heap_object) from [<802e8774>] (__check_object_size+0x17c/0x1e0)
> [    3.005996]  r8:00000001 r7:811eb001 r6:00000001 r5:00000801 r4:811ea800
> [    3.006555] [<802e85f8>] (__check_object_size) from [<804dedf0>] (kfifo_copy_to_user+0xa8/0x1b8)
> [    3.007290]  r7:811ea800 r6:7edc2593 r5:00001000 r4:00000801
> [    3.007766] [<804ded48>] (kfifo_copy_to_user) from [<804def50>] (__kfifo_to_user+0x50/0x70)
> [    3.008418]  r9:810e2000 r8:00001000 r7:7edc1d94 r6:810e3e98 r5:00001000 r4:811d584c
> [    3.009031] [<804def00>] (__kfifo_to_user) from [<80579a58>] (snoop_file_read+0xb0/0xf8)
> [    3.009683]  r6:00000001 r5:810e3e98 r4:811d586c
> [    3.010055] [<805799a8>] (snoop_file_read) from [<802ee040>] (vfs_read+0xbc/0x328)
> [    3.010686]  r8:00001000 r7:805799a8 r6:00000001 r5:810e3f68 r4:819a2cc0
> [    3.011182] [<802edf84>] (vfs_read) from [<802ee740>] (ksys_read+0x70/0xfc)
> [    3.011762]  r10:00000003 r9:810e2000 r8:801002c4 r7:00000000 r6:00000000 r5:819a2cc0
> [    3.012451]  r4:819a2cc0
> [    3.012677] [<802ee6d0>] (ksys_read) from [<802ee7e4>] (sys_read+0x18/0x1c)
> [    3.013261]  r7:00000003 r6:00006f2c r5:014fc580 r4:00000003
> [    3.013724] [<802ee7cc>] (sys_read) from [<80100080>] (ret_fast_syscall+0x0/0x48)
> [    3.014330] Exception stack(0x810e3fa8 to 0x810e3ff0)
> [    3.014767] 3fa0:                   00000003 014fc580 00000003 7edc1d94 00001000 00000000
> [    3.015379] 3fc0: 00000003 014fc580 00006f2c 00000003 7edc2f10 00000000 00000001 00000000
> [    3.016030] 3fe0: 000000ac 7edc1d18 0002c174 0002c190
> [    3.016641] Code: e1cd40fc e58dc000 e59f0024 ebfff741 (e7f001f2)
> [    3.017733] ---[ end trace d9f7e472f48076c9 ]---
> [    3.018380] Kernel panic - not syncing: Fatal exception
> [    3.019190] ---[ end Kernel panic - not syncing: Fatal exception ]---

Can you please trim this a bit? See the discussion of backtraces here:

https://docs.kernel.org/process/submitting-patches.html#explanation-body

> 
> 
> This is reproducible on AST2500 BMC (dual-core ARM Cortex-A7) when

The AST2500 has a (single-core) ARM1176JZS, not a dual-core Cortex-A7.

> BIOS floods POST codes while userspace concurrently reads
> /dev/aspeed-lpc-snoop0.
> 
> Fix by:
> 
> 1. Clamping 'count' to SNOOP_FIFO_SIZE in snoop_file_read() so that
>    copy_to_user() can never exceed the slab object boundary regardless
>    of kfifo pointer state.

I don't think we shouldn't be double-accounting for the bug. Let's just
get the locking right given the observation that we have multiple
consumers.

> 
> 2. Protecting the kfifo_skip() + kfifo_put() sequence in
>    put_fifo_with_discard() and the kfifo_to_user() call in
>    snoop_file_read() with a spinlock, ensuring pointer updates are
>    atomic with respect to concurrent access and restoring the
>    single-writer invariant for each pointer.

This appears sensible.

> 
>    put_fifo_with_discard() is only called from hardirq context where
>    interrupts are already disabled, so plain spin_lock/spin_unlock is
>    used. snoop_file_read() runs in process context and must use
>    spin_lock_irqsave to prevent deadlock with the IRQ handler.

We only need _irqsave for generic code that runs in either context, but
the fops callbacks are never run in irq context, so we don't have that
ambiguity. You could make other choices here, however _irqsave isn't
wrong.

> 
> Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>

Can you please add a Fixes: tag?

> ---
> Andrew,
> 
> You're right — __kfifo_to_user() clamps to (in - out). The issue is
> that (in - out) itself is corrupted by a race in put_fifo_with_discard().
> 
> The function calls kfifo_skip() (out++) then kfifo_put() (in++) without
> synchronization. On SMP ARM, the reader observes fresh 'in' (barrier in
> kfifo_put) but stale 'out' (no barrier in kfifo_skip), making (in - out)
> exceed the buffer size. The clamp then passes through the corrupted value.
> 
> Reproduced on QEMU ast2500-evb by injecting POST codes and simulating
> the race outcome. 
> 

Can you provide more details? The 2500 is single-core, so is either
executing in interrupt context or not, and the values should be
consistent after completing the interrupt.

> Also observed intermittently on physical dual-core
> AST2600 under heavy POST code traffic.

The AST2600 has a dual-core Cortex-A7, so your bug makes more sense to
me there...

Andrew

[PATCH v3] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

put_fifo_with_discard() acts as both producer and consumer on the kfifo:
it calls kfifo_skip() (advances out) and kfifo_put() (advances in) from
the IRQ handler without synchronizing with snoop_file_read(), which also
consumes via kfifo_to_user(). On SMP systems this concurrent access can
leave (in - out) larger than the ring buffer, so __kfifo_to_user()'s clamp
to (in - out) is ineffective and kfifo_copy_to_user() can attempt a
copy_to_user() past the kmalloc-2k backing store:

  usercopy: Kernel memory exposure attempt detected from SLUB object
  'kmalloc-2k' (offset 0, size 2049)!
  kernel BUG at mm/usercopy.c:99!
  Call trace:
   usercopy_abort
   __check_heap_object
   __check_object_size
   kfifo_copy_to_user
   __kfifo_to_user
   snoop_file_read
   vfs_read

Reproduced on ast2600-evb (dual-core ARM Cortex-A7) when the host floods
POST codes while userspace reads /dev/aspeed-lpc-snoop0.

Serialize kfifo access with a per-channel spinlock: use spin_lock()/
spin_unlock() in put_fifo_with_discard() (hardirq only) and
spin_lock_irq()/spin_unlock_irq() around kfifo_to_user() in
snoop_file_read().

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Cc: stable@vger.kernel.org
Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
Andrew,

Thanks for the review.

> The AST2500 has a (single-core) ARM1176JZS

Corrected in v3.

> Don't double-account for the bug

Agreed — the spinlock eliminates the unsynchronized window that
produces the inconsistent pointer state. Clamp removed.

> _irqsave isn't wrong

Changed to spin_lock_irq — fops callbacks always enter with
interrupts enabled.

> Can you provide more details? The 2500 is single-core

The issue was observed on physical AST2600 (dual-core Cortex-A7)
in production under heavy POST code traffic during concurrent
userspace reads. Since the x86 host does not model ARM weak memory
ordering, the race cannot be reproduced naturally in QEMU. The test
module adjusts kfifo pointers to reproduce the post-race state for
deterministic validation.

> AST2600 has a dual-core Cortex-A7, so your bug makes more sense there

Yes, the issue is intermittently observed on production AST2600.

Changes since v2:
- Dropped count clamp
- spin_lock_irqsave -> spin_lock_irq in snoop_file_read
- Fixed platform: AST2600 (dual-core Cortex-A7)
- Trimmed backtrace
- Added Fixes tag

 drivers/soc/aspeed/aspeed-lpc-snoop.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index eceeaf8df..ef6697a42 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -60,6 +60,7 @@ struct aspeed_lpc_snoop_model_data {
 
 struct aspeed_lpc_snoop_channel {
 	struct kfifo		fifo;
+	spinlock_t		lock;
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
 };
@@ -93,7 +94,11 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 		if (ret == -ERESTARTSYS)
 			return -EINTR;
 	}
+
+	spin_lock_irq(&chan->lock);
 	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
+	spin_unlock_irq(&chan->lock);
+
 	if (ret)
 		return ret;
 
@@ -121,9 +126,11 @@ static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
 {
 	if (!kfifo_initialized(&chan->fifo))
 		return;
+	spin_lock(&chan->lock);
 	if (kfifo_is_full(&chan->fifo))
 		kfifo_skip(&chan->fifo);
 	kfifo_put(&chan->fifo, val);
+	spin_unlock(&chan->lock);
 	wake_up_interruptible(&chan->wq);
 }
 
@@ -192,6 +199,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		of_device_get_match_data(dev);
 
 	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
+	spin_lock_init(&lpc_snoop->chan[channel].lock);
 	/* Create FIFO datastructure */
 	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
 			 SNOOP_FIFO_SIZE, GFP_KERNEL);
-- 
2.43.0

Re: [PATCH v3] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

Hi Karthikeyan,

On Wed, 2026-05-27 at 17:59 +0000, Karthikeyan KS wrote:
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index eceeaf8df..ef6697a42 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -60,6 +60,7 @@ struct aspeed_lpc_snoop_model_data {
>  
>  struct aspeed_lpc_snoop_channel {
>  	struct kfifo		fifo;
> +	spinlock_t		lock;
>  	wait_queue_head_t	wq;
>  	struct miscdevice	miscdev;
>  };
> @@ -93,7 +94,11 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
>  		if (ret == -ERESTARTSYS)
>  			return -EINTR;
>  	}
> +
> +	spin_lock_irq(&chan->lock);
>  	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
> +	spin_unlock_irq(&chan->lock);

This seems inappropriate and I expect is flagged if you compile with
CONFIG_PROVE_LOCKING=y or CONFIG_DEBUG_ATOMIC_SLEEP=y. I suggest both
if you're not already.

Further, I hit conflicts when applying your change on v7.1-rc5. Can you
please ensure you develop, build and test on recent releases.

Thanks,

Andrew

[PATCH v4] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

put_fifo_with_discard() acts as both producer and consumer on the kfifo:
it calls kfifo_skip() (advances out) and kfifo_put() (advances in) from
the IRQ handler without synchronizing with snoop_file_read(), which also
consumes via kfifo_to_user(). On SMP systems this concurrent access can
leave (in - out) larger than the ring buffer, so __kfifo_to_user()'s clamp
to (in - out) is ineffective and kfifo_copy_to_user() can attempt a
copy_to_user() past the kmalloc-2k backing store:

  usercopy: Kernel memory exposure attempt detected from SLUB object
  'kmalloc-2k' (offset 0, size 2049)!
  kernel BUG at mm/usercopy.c!
  Call trace:
   usercopy_abort
   __check_heap_object
   __check_object_size
   kfifo_copy_to_user
   __kfifo_to_user
   snoop_file_read
   vfs_read


Serialize kfifo access with a per-channel spinlock. copy_to_user()
runs after dropping the lock, since it may sleep on a page fault.

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Cc: stable@vger.kernel.org
Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
Andrew,

Thanks for the review.

> This seems inappropriate and I expect is flagged if you compile with
> CONFIG_PROVE_LOCKING=y or CONFIG_DEBUG_ATOMIC_SLEEP=y

v4 drains the kfifo into a kernel buffer via kfifo_out() under
the lock, then performs copy_to_user() after dropping it.
(cf. drivers/gpio/gpiolib-cdev.c, which drains under its event lock
and copies outside it.)

> ensure you develop, build and test on recent releases

Tested on both v7.1-rc5 and v7.1-rc6 with PROVE_LOCKING,
DEBUG_ATOMIC_SLEEP and HARDENED_USERCOPY enabled: read path
round-trips correctly, no lockdep splats, no atomic-sleep
warnings, no usercopy aborts.

Changes since v3:
- Replaced kfifo_to_user() with kfifo_out() + copy_to_user()
  to avoid sleeping under spinlock
- Rebased onto v7.1-rc6

 drivers/soc/aspeed/aspeed-lpc-snoop.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index b03310c0830d..0fe463020e25 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -74,6 +74,7 @@ struct aspeed_lpc_snoop_channel_cfg {
 struct aspeed_lpc_snoop_channel {
 	const struct aspeed_lpc_snoop_channel_cfg *cfg;
 	bool enabled;
+	spinlock_t		lock;
 	struct kfifo		fifo;
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
@@ -115,6 +116,7 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 {
 	struct aspeed_lpc_snoop_channel *chan = snoop_file_to_chan(file);
 	unsigned int copied;
+	u8 *buf;
 	int ret = 0;
 
 	if (kfifo_is_empty(&chan->fifo)) {
@@ -125,11 +127,22 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 		if (ret == -ERESTARTSYS)
 			return -EINTR;
 	}
-	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
-	if (ret)
-		return ret;
 
-	return copied;
+	buf = kmalloc(SNOOP_FIFO_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	spin_lock_irq(&chan->lock);
+	copied = kfifo_out(&chan->fifo, buf,
+			   min_t(size_t, count, SNOOP_FIFO_SIZE));
+	spin_unlock_irq(&chan->lock);
+
+	ret = copied;
+	if (copied && copy_to_user(buffer, buf, copied))
+		ret = -EFAULT;
+
+	kfree(buf);
+	return ret;
 }
 
 static __poll_t snoop_file_poll(struct file *file,
@@ -153,9 +166,11 @@ static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
 {
 	if (!kfifo_initialized(&chan->fifo))
 		return;
+	spin_lock(&chan->lock);
 	if (kfifo_is_full(&chan->fifo))
 		kfifo_skip(&chan->fifo);
 	kfifo_put(&chan->fifo, val);
+	spin_unlock(&chan->lock);
 	wake_up_interruptible(&chan->wq);
 }
 
@@ -228,6 +243,7 @@ static int aspeed_lpc_enable_snoop(struct device *dev,
 		return -EBUSY;
 
 	init_waitqueue_head(&channel->wq);
+	spin_lock_init(&channel->lock);
 
 	channel->cfg = cfg;
 	channel->miscdev.minor = MISC_DYNAMIC_MINOR;
-- 
2.43.0

Re: [PATCH v4] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

Hi Karthikeyan,

On Mon, 2026-06-01 at 12:52 +0000, Karthikeyan KS wrote:
> put_fifo_with_discard() acts as both producer and consumer on the kfifo:
> it calls kfifo_skip() (advances out) and kfifo_put() (advances in) from
> the IRQ handler without synchronizing with snoop_file_read(), which also
> consumes via kfifo_to_user(). On SMP systems this concurrent access can
> leave (in - out) larger than the ring buffer, so __kfifo_to_user()'s clamp
> to (in - out) is ineffective and kfifo_copy_to_user() can attempt a
> copy_to_user() past the kmalloc-2k backing store:
> 
>   usercopy: Kernel memory exposure attempt detected from SLUB object
>   'kmalloc-2k' (offset 0, size 2049)!
>   kernel BUG at mm/usercopy.c!
>   Call trace:
>    usercopy_abort
>    __check_heap_object
>    __check_object_size
>    kfifo_copy_to_user
>    __kfifo_to_user
>    snoop_file_read
>    vfs_read
> 
> 
> Serialize kfifo access with a per-channel spinlock. copy_to_user()
> runs after dropping the lock, since it may sleep on a page fault.
> 
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
> Cc: stable@vger.kernel.org
> Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
> ---
> Andrew,
> 
> Thanks for the review.
> 
> > This seems inappropriate and I expect is flagged if you compile with
> > CONFIG_PROVE_LOCKING=y or CONFIG_DEBUG_ATOMIC_SLEEP=y
> 
> v4 drains the kfifo into a kernel buffer via kfifo_out() under
> the lock, then performs copy_to_user() after dropping it.
> (cf. drivers/gpio/gpiolib-cdev.c, which drains under its event lock
> and copies outside it.)
> 
> > ensure you develop, build and test on recent releases
> 
> Tested on both v7.1-rc5 and v7.1-rc6 with PROVE_LOCKING,
> DEBUG_ATOMIC_SLEEP and HARDENED_USERCOPY enabled: read path
> round-trips correctly, no lockdep splats, no atomic-sleep
> warnings, no usercopy aborts.
> 
> Changes since v3:
> - Replaced kfifo_to_user() with kfifo_out() + copy_to_user()
>   to avoid sleeping under spinlock
> - Rebased onto v7.1-rc6
> 
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index b03310c0830d..0fe463020e25 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -74,6 +74,7 @@ struct aspeed_lpc_snoop_channel_cfg {
>  struct aspeed_lpc_snoop_channel {
>  	const struct aspeed_lpc_snoop_channel_cfg *cfg;
>  	bool enabled;
> +	spinlock_t		lock;
>  	struct kfifo		fifo;
>  	wait_queue_head_t	wq;
>  	struct miscdevice	miscdev;
> @@ -115,6 +116,7 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
>  {
>  	struct aspeed_lpc_snoop_channel *chan = snoop_file_to_chan(file);
>  	unsigned int copied;
> +	u8 *buf;

Can use the cleanup helpers again here:

   u8 *buf __free(kfree) = NULL;

>  	int ret = 0;
>  
>  	if (kfifo_is_empty(&chan->fifo)) {
> @@ -125,11 +127,22 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
>  		if (ret == -ERESTARTSYS)
>  			return -EINTR;
>  	}
> -	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
> -	if (ret)
> -		return ret;
>  
> -	return copied;
> +	buf = kmalloc(SNOOP_FIFO_SIZE, GFP_KERNEL);

I expect using count clamped to SNOOP_FIFO_SIZE might be a better
option here? The clamp below can be moved here.

I'm not enamoured with the bounce buffer, but I guess it solves the
problem.

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	spin_lock_irq(&chan->lock);
> +	copied = kfifo_out(&chan->fifo, buf,
> +			   min_t(size_t, count, SNOOP_FIFO_SIZE));

This is handled by kfifo_out() as discussed previously, but also see
the above. You may want to check that count doesn't exceed UINT_MAX
though, in the event that SIZE_MAX > UINT_MAX.

> +	spin_unlock_irq(&chan->lock);

Recently the kernel gained cleanup helpers. scoped_guard() would be
handy here, however the kfifo API also provides kfifo_out_spinlocked().
I'd use that as it is at least idiomatic.

> +
> +	ret = copied;
> +	if (copied && copy_to_user(buffer, buf, copied))
> +		ret = -EFAULT;
> +
> +	kfree(buf);
> +	return ret;
>  }
>  
>  static __poll_t snoop_file_poll(struct file *file,
> @@ -153,9 +166,11 @@ static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
>  {
>  	if (!kfifo_initialized(&chan->fifo))
>  		return;
> +	spin_lock(&chan->lock);
>  	if (kfifo_is_full(&chan->fifo))
>  		kfifo_skip(&chan->fifo);
>  	kfifo_put(&chan->fifo, val);
> +	spin_unlock(&chan->lock);

I prefer we use scoped_guard() here.

>  	wake_up_interruptible(&chan->wq);
>  }
>  
> @@ -228,6 +243,7 @@ static int aspeed_lpc_enable_snoop(struct device *dev,
>  		return -EBUSY;
>  
>  	init_waitqueue_head(&channel->wq);
> +	spin_lock_init(&channel->lock);
>  
>  	channel->cfg = cfg;
>  	channel->miscdev.minor = MISC_DYNAMIC_MINOR;

[PATCH v5] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

put_fifo_with_discard() acts as both producer and consumer on the kfifo:
it calls kfifo_skip() (advances out) and kfifo_put() (advances in) from
the IRQ handler without synchronizing with snoop_file_read(), which also
consumes via kfifo_to_user(). On SMP systems this concurrent access can
leave (in - out) larger than the ring buffer, so __kfifo_to_user()'s clamp
to (in - out) is ineffective and kfifo_copy_to_user() can attempt a
copy_to_user() past the kmalloc-2k backing store:

  usercopy: Kernel memory exposure attempt detected from SLUB object
  'kmalloc-2k' (offset 0, size 2049)!
  kernel BUG at mm/usercopy.c!
  Call trace:
   usercopy_abort
   __check_heap_object
   __check_object_size
   kfifo_copy_to_user
   __kfifo_to_user
   snoop_file_read
   vfs_read

Serialize kfifo access with a per-channel spinlock. The reader drains
into a bounce buffer under the lock with kfifo_out_spinlocked() and then
copies to userspace after dropping it, since copy_to_user() may sleep on
a page fault.

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Cc: stable@vger.kernel.org
Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
Andrew,

Thanks for the review.

Changes since v4:
- Use __free(kfree) for automatic cleanup
- Allocate clamped count instead of full SNOOP_FIFO_SIZE
- Use kfifo_out_spinlocked() in snoop_file_read
- Use scoped_guard(spinlock) in put_fifo_with_discard

 drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index b03310c0830d..c9c87a794228 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/dev_printk.h>
 #include <linux/interrupt.h>
@@ -74,6 +75,7 @@ struct aspeed_lpc_snoop_channel_cfg {
 struct aspeed_lpc_snoop_channel {
 	const struct aspeed_lpc_snoop_channel_cfg *cfg;
 	bool enabled;
+	spinlock_t		lock;    /* serialises @fifo: irq producer vs reader */
 	struct kfifo		fifo;
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
@@ -114,6 +116,7 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 				size_t count, loff_t *ppos)
 {
 	struct aspeed_lpc_snoop_channel *chan = snoop_file_to_chan(file);
+	u8 *buf __free(kfree) = NULL;
 	unsigned int copied;
 	int ret = 0;
 
@@ -125,9 +128,16 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 		if (ret == -ERESTARTSYS)
 			return -EINTR;
 	}
-	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
-	if (ret)
-		return ret;
+
+	count = min_t(size_t, count, SNOOP_FIFO_SIZE);
+
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	copied = kfifo_out_spinlocked(&chan->fifo, buf, count, &chan->lock);
+	if (copied && copy_to_user(buffer, buf, copied))
+		return -EFAULT;
 
 	return copied;
 }
@@ -153,9 +163,11 @@ static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
 {
 	if (!kfifo_initialized(&chan->fifo))
 		return;
-	if (kfifo_is_full(&chan->fifo))
-		kfifo_skip(&chan->fifo);
-	kfifo_put(&chan->fifo, val);
+	scoped_guard(spinlock, &chan->lock) {
+		if (kfifo_is_full(&chan->fifo))
+			kfifo_skip(&chan->fifo);
+		kfifo_put(&chan->fifo, val);
+	}
 	wake_up_interruptible(&chan->wq);
 }
 
@@ -228,6 +240,7 @@ static int aspeed_lpc_enable_snoop(struct device *dev,
 		return -EBUSY;
 
 	init_waitqueue_head(&channel->wq);
+	spin_lock_init(&channel->lock);
 
 	channel->cfg = cfg;
 	channel->miscdev.minor = MISC_DYNAMIC_MINOR;
-- 
2.43.0

Re: [PATCH v5] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

Hi Karthikeyan,

On Wed, 2026-06-10 at 17:23 +0000, Karthikeyan KS wrote:
> put_fifo_with_discard() acts as both producer and consumer on the kfifo:
> it calls kfifo_skip() (advances out) and kfifo_put() (advances in) from
> the IRQ handler without synchronizing with snoop_file_read(), which also
> consumes via kfifo_to_user(). On SMP systems this concurrent access can
> leave (in - out) larger than the ring buffer, so __kfifo_to_user()'s clamp
> to (in - out) is ineffective and kfifo_copy_to_user() can attempt a
> copy_to_user() past the kmalloc-2k backing store:
> 
>   usercopy: Kernel memory exposure attempt detected from SLUB object
>   'kmalloc-2k' (offset 0, size 2049)!
>   kernel BUG at mm/usercopy.c!
>   Call trace:
>    usercopy_abort
>    __check_heap_object
>    __check_object_size
>    kfifo_copy_to_user
>    __kfifo_to_user
>    snoop_file_read
>    vfs_read
> 
> Serialize kfifo access with a per-channel spinlock. The reader drains
> into a bounce buffer under the lock with kfifo_out_spinlocked() and then
> copies to userspace after dropping it, since copy_to_user() may sleep on
> a page fault.
> 
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
> Cc: stable@vger.kernel.org
> Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
> ---
> Andrew,
> 
> Thanks for the review.
> 
> Changes since v4:
> - Use __free(kfree) for automatic cleanup
> - Allocate clamped count instead of full SNOOP_FIFO_SIZE
> - Use kfifo_out_spinlocked() in snoop_file_read
> - Use scoped_guard(spinlock) in put_fifo_with_discard
> 
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index b03310c0830d..c9c87a794228 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/dev_printk.h>
>  #include <linux/interrupt.h>
> @@ -74,6 +75,7 @@ struct aspeed_lpc_snoop_channel_cfg {
>  struct aspeed_lpc_snoop_channel {
>  	const struct aspeed_lpc_snoop_channel_cfg *cfg;
>  	bool enabled;
> +	spinlock_t		lock;    /* serialises @fifo: irq producer vs reader */

I'd prefer we avoid trailing comments, which it seems you've added this
time around. Since you did that ...

>  	struct kfifo		fifo;

... in this specific case we can improve on the comment, with:

   struct kfifo fifo __guarded_by(&lock);

More details here:

   https://docs.kernel.org/dev-tools/context-analysis.html

Adding a change along these lines currently produces:

   ../drivers/soc/aspeed/aspeed-lpc-snoop.c:164:32: warning: reading variable 'fifo' requires holding spinlock '&aspeed_lpc_snoop_channel::lock' [-Wthread-safety-analysis]
     164 |         if (!kfifo_initialized(&chan->fifo))
         |                                       ^

I ended up applying this on top of your patch:

   diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
   index b35d74592964..9cba7be8c395 100644
   --- a/drivers/soc/aspeed/Makefile
   +++ b/drivers/soc/aspeed/Makefile
   @@ -4,3 +4,5 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)          += aspeed-lpc-snoop.o
    obj-$(CONFIG_ASPEED_UART_ROUTING)      += aspeed-uart-routing.o
    obj-$(CONFIG_ASPEED_P2A_CTRL)          += aspeed-p2a-ctrl.o
    obj-$(CONFIG_ASPEED_SOCINFO)           += aspeed-socinfo.o
   +
   +CONTEXT_ANALYSIS_aspeed-lpc-snoop.o    := y
   diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
   index 9165a543a250..7fa1a345acac 100644
   --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
   +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
   @@ -75,8 +75,8 @@ struct aspeed_lpc_snoop_channel_cfg {
    struct aspeed_lpc_snoop_channel {
           const struct aspeed_lpc_snoop_channel_cfg *cfg;
           bool enabled;
   -       spinlock_t              lock;    /* serialises @fifo: irq producer vs reader */
   -       struct kfifo            fifo;
   +       spinlock_t              lock;
   +       struct kfifo            fifo __guarded_by(&lock);
           wait_queue_head_t       wq;
           struct miscdevice       miscdev;
    };
   @@ -161,9 +161,9 @@ static const struct file_operations snoop_fops = {
    /* Save a byte to a FIFO and discard the oldest byte if FIFO is full */
    static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
    {
   -       if (!kfifo_initialized(&chan->fifo))
   -               return;
           scoped_guard(spinlock, &chan->lock) {
   +               if (!kfifo_initialized(&chan->fifo))
   +                       return;
                   if (kfifo_is_full(&chan->fifo))
                           kfifo_skip(&chan->fifo);
                   kfifo_put(&chan->fifo, val);
   @@ -240,7 +240,6 @@ static int aspeed_lpc_enable_snoop(struct device *dev,
                   return -EBUSY;
    
           init_waitqueue_head(&channel->wq);
   -       spin_lock_init(&channel->lock);
    
           channel->cfg = cfg;
           channel->miscdev.minor = MISC_DYNAMIC_MINOR;
   @@ -252,9 +251,11 @@ static int aspeed_lpc_enable_snoop(struct device *dev,
           if (!channel->miscdev.name)
                   return -ENOMEM;
    
   -       rc = kfifo_alloc(&channel->fifo, SNOOP_FIFO_SIZE, GFP_KERNEL);
   -       if (rc)
   -               return rc;
   +       scoped_guard(spinlock_init, &channel->lock) {
   +               rc = kfifo_alloc(&channel->fifo, SNOOP_FIFO_SIZE, GFP_KERNEL);
   +               if (rc)
   +                       return rc;
   +       }
    
           rc = misc_register(&channel->miscdev);
           if (rc)
   
I prefer that we add the annotation as the compiler analysis provides
some comfort in contrast to the comment.

Otherwise, the rest of the fix seems okay to me.

Andrew

>  	wait_queue_head_t	wq;
>  	struct miscdevice	miscdev;
> @@ -114,6 +116,7 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
>  				size_t count, loff_t *ppos)
>  {
>  	struct aspeed_lpc_snoop_channel *chan = snoop_file_to_chan(file);
> +	u8 *buf __free(kfree) = NULL;
>  	unsigned int copied;
>  	int ret = 0;
>  
> @@ -125,9 +128,16 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
>  		if (ret == -ERESTARTSYS)
>  			return -EINTR;
>  	}
> -	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
> -	if (ret)
> -		return ret;
> +
> +	count = min_t(size_t, count, SNOOP_FIFO_SIZE);
> +
> +	buf = kmalloc(count, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	copied = kfifo_out_spinlocked(&chan->fifo, buf, count, &chan->lock);
> +	if (copied && copy_to_user(buffer, buf, copied))
> +		return -EFAULT;
>  
>  	return copied;
>  }
> @@ -153,9 +163,11 @@ static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
>  {
>  	if (!kfifo_initialized(&chan->fifo))
>  		return;
> -	if (kfifo_is_full(&chan->fifo))
> -		kfifo_skip(&chan->fifo);
> -	kfifo_put(&chan->fifo, val);
> +	scoped_guard(spinlock, &chan->lock) {
> +		if (kfifo_is_full(&chan->fifo))
> +			kfifo_skip(&chan->fifo);
> +		kfifo_put(&chan->fifo, val);
> +	}
>  	wake_up_interruptible(&chan->wq);
>  }
>  
> @@ -228,6 +240,7 @@ static int aspeed_lpc_enable_snoop(struct device *dev,
>  		return -EBUSY;
>  
>  	init_waitqueue_head(&channel->wq);
> +	spin_lock_init(&channel->lock);
>  
>  	channel->cfg = cfg;
>  	channel->miscdev.minor = MISC_DYNAMIC_MINOR;

Re: [PATCH v5] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

On Thu, 2026-06-11 at 23:01 +0530, karthikeyan K S wrote:
> Thanks Andrew. The __guarded_by annotation and context analysis integration
> look good, I wasn't aware of that infrastructure.
> Thanks for applying those changes on top.

Sorry, on reflection I chose my words poorly there. I applied that
patch I pasted on top as an experiment on my end. I haven't yet added
your patch to the fixes branch.

Do you mind integrating that rework, testing, and then sending the
result?

Cheers,

Andrew

Re: [PATCH v5] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

Hi Andrew,

Thanks Andrew. The __guarded_by annotation and context analysis integration look good, I wasn't aware of that infrastructure.
Thanks for applying the fix-up on top.

Thanks,
Karthikeyan

[PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

put_fifo_with_discard() acts as both producer and consumer on the kfifo:
it calls kfifo_skip() (advances out) and kfifo_put() (advances in) from
the IRQ handler without synchronizing with snoop_file_read(), which also
consumes via kfifo_to_user(). On SMP systems this concurrent access can
leave (in - out) larger than the ring buffer, so __kfifo_to_user()'s clamp
to (in - out) is ineffective and kfifo_copy_to_user() can attempt a
copy_to_user() past the kmalloc-2k backing store:

  usercopy: Kernel memory exposure attempt detected from SLUB object
  'kmalloc-2k' (offset 0, size 2049)!
  kernel BUG at mm/usercopy.c!
  Call trace:
   usercopy_abort
   __check_heap_object
   __check_object_size
   kfifo_copy_to_user
   __kfifo_to_user
   snoop_file_read
   vfs_read

Serialize kfifo access with a per-channel spinlock shared between the
IRQ handler (producer) and the file reader (consumer).  Annotate @fifo
with __guarded_by(&lock) and opt the driver into context analysis so the
compiler enforces that all fifo access holds the lock.

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
 drivers/soc/aspeed/Makefile           |  1 +
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 38 ++++++++++++++++++---------
 2 files changed, 27 insertions(+), 12 deletions(-)

Andrew,

Thanks for the review.

Changes since v5:
- Annotate @fifo with __guarded_by(&lock) instead of a comment
- Move kfifo_initialized() check inside scoped_guard(spinlock, &chan->lock)
  in put_fifo_with_discard()
- Replace spin_lock_init() with scoped_guard(spinlock_init, &channel->lock)
  around kfifo_alloc() in aspeed_lpc_enable_snoop()
- Enable CONTEXT_ANALYSIS for this driver in drivers/soc/aspeed/Makefile

Dropped Cc: stable — the fix uses cleanup.h/context-analysis idioms absent
from LTS; I'll send adapted backports to stable@ once this is in mainline.

Tested on ast2600-evb (QEMU): clang-22 with CONFIG_WARN_CONTEXT_ANALYSIS=y
shows no context-analysis warnings; PROVE_LOCKING, DEBUG_ATOMIC_SLEEP and
HARDENED_USERCOPY show no splats. Overflow reproduced via a fault-injection
module forcing the post-race (in - out) state (QEMU doesn't model the ARM
ordering that triggers it in the field): unpatched panics, patched returns
cleanly.

Thanks,
Karthikeyan

diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..b5188dcde37a 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
 obj-$(CONFIG_ASPEED_P2A_CTRL)		+= aspeed-p2a-ctrl.o
 obj-$(CONFIG_ASPEED_SOCINFO)		+= aspeed-socinfo.o
+CONTEXT_ANALYSIS_aspeed-lpc-snoop.o	:= y
diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index b03310c0830d..7fa1a345acac 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/dev_printk.h>
 #include <linux/interrupt.h>
@@ -74,7 +75,8 @@ struct aspeed_lpc_snoop_channel_cfg {
 struct aspeed_lpc_snoop_channel {
 	const struct aspeed_lpc_snoop_channel_cfg *cfg;
 	bool enabled;
-	struct kfifo		fifo;
+	spinlock_t		lock;
+	struct kfifo		fifo __guarded_by(&lock);
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
 };
@@ -114,6 +116,7 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 				size_t count, loff_t *ppos)
 {
 	struct aspeed_lpc_snoop_channel *chan = snoop_file_to_chan(file);
+	u8 *buf __free(kfree) = NULL;
 	unsigned int copied;
 	int ret = 0;
 
@@ -125,9 +128,16 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 		if (ret == -ERESTARTSYS)
 			return -EINTR;
 	}
-	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
-	if (ret)
-		return ret;
+
+	count = min_t(size_t, count, SNOOP_FIFO_SIZE);
+
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	copied = kfifo_out_spinlocked(&chan->fifo, buf, count, &chan->lock);
+	if (copied && copy_to_user(buffer, buf, copied))
+		return -EFAULT;
 
 	return copied;
 }
@@ -151,11 +161,13 @@ static const struct file_operations snoop_fops = {
 /* Save a byte to a FIFO and discard the oldest byte if FIFO is full */
 static void put_fifo_with_discard(struct aspeed_lpc_snoop_channel *chan, u8 val)
 {
-	if (!kfifo_initialized(&chan->fifo))
-		return;
-	if (kfifo_is_full(&chan->fifo))
-		kfifo_skip(&chan->fifo);
-	kfifo_put(&chan->fifo, val);
+	scoped_guard(spinlock, &chan->lock) {
+		if (!kfifo_initialized(&chan->fifo))
+			return;
+		if (kfifo_is_full(&chan->fifo))
+			kfifo_skip(&chan->fifo);
+		kfifo_put(&chan->fifo, val);
+	}
 	wake_up_interruptible(&chan->wq);
 }
 
@@ -239,9 +251,11 @@ static int aspeed_lpc_enable_snoop(struct device *dev,
 	if (!channel->miscdev.name)
 		return -ENOMEM;
 
-	rc = kfifo_alloc(&channel->fifo, SNOOP_FIFO_SIZE, GFP_KERNEL);
-	if (rc)
-		return rc;
+	scoped_guard(spinlock_init, &channel->lock) {
+		rc = kfifo_alloc(&channel->fifo, SNOOP_FIFO_SIZE, GFP_KERNEL);
+		if (rc)
+			return rc;
+	}
 
 	rc = misc_register(&channel->miscdev);
 	if (rc)
--
2.43.0

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

On Fri, 2026-06-12 at 19:07 +0000, Karthikeyan KS wrote:
> put_fifo_with_discard() acts as both producer and consumer on the kfifo:
> it calls kfifo_skip() (advances out) and kfifo_put() (advances in) from
> the IRQ handler without synchronizing with snoop_file_read(), which also
> consumes via kfifo_to_user(). On SMP systems this concurrent access can
> leave (in - out) larger than the ring buffer, so __kfifo_to_user()'s clamp
> to (in - out) is ineffective and kfifo_copy_to_user() can attempt a
> copy_to_user() past the kmalloc-2k backing store:
> 
>   usercopy: Kernel memory exposure attempt detected from SLUB object
>   'kmalloc-2k' (offset 0, size 2049)!
>   kernel BUG at mm/usercopy.c!
>   Call trace:
>    usercopy_abort
>    __check_heap_object
>    __check_object_size
>    kfifo_copy_to_user
>    __kfifo_to_user
>    snoop_file_read
>    vfs_read
> 
> Serialize kfifo access with a per-channel spinlock shared between the
> IRQ handler (producer) and the file reader (consumer).  Annotate @fifo
> with __guarded_by(&lock) and opt the driver into context analysis so the
> compiler enforces that all fifo access holds the lock.
> 
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
> Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
> ---
>  drivers/soc/aspeed/Makefile           |  1 +
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 38 ++++++++++++++++++---------
>  2 files changed, 27 insertions(+), 12 deletions(-)
> 
> Andrew,
> 
> Thanks for the review.
> 
> Changes since v5:
> - Annotate @fifo with __guarded_by(&lock) instead of a comment
> - Move kfifo_initialized() check inside scoped_guard(spinlock, &chan->lock)
>   in put_fifo_with_discard()
> - Replace spin_lock_init() with scoped_guard(spinlock_init, &channel->lock)
>   around kfifo_alloc() in aspeed_lpc_enable_snoop()
> - Enable CONTEXT_ANALYSIS for this driver in drivers/soc/aspeed/Makefile
> 
> Dropped Cc: stable — the fix uses cleanup.h/context-analysis idioms absent
> from LTS; I'll send adapted backports to stable@ once this is in mainline.
> 
> Tested on ast2600-evb (QEMU): 
> 

Can you describe the specific steps you used to test this under qemu?
I'm interested in reproducing your efforts here.

Andrew

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

Hi Andrew,

Happy to. Short version: ast2600-evb can't hit the SMP timing window,
so I reproduce each missing piece deliberately. The driver code under
test is unmodified -- only the stimulus and the post-race state are
injected. Stock qemu-system-arm (Debian 8.2.2), no QEMU changes.

Three obstacles, and what I did about each:

1. No BIOS to emit POST codes -- an injection module stages bytes into
   the snoop registers via the LPC syscon regmap (SNPWDR + the HICR6
   data-ready bit).

2. QEMU doesn't raise the snoop IRQ for those writes -- after staging,
   the module dispatches it in software with
   generic_handle_irq_safe(sdev->irq), which runs the driver's real
   aspeed_lpc_snoop_irq() -> put_fifo_with_discard() path.

3. The SMP race won't trigger under TCG -- so I reconstruct its outcome
   instead: force the channel-0 kfifo to in=4097, out=1, i.e.
   (in - out) = 4096 > the 2048-byte ring, the exact state a reader
   observes inside the window.

One caveat so it isn't misread: step 3 writes in/out directly, so it
bypasses the new lock. The patched run therefore shows the read path no
longer turns a corrupt (in - out) into a usercopy overflow; the lock's
job of preventing that state is the SPSC argument from the commit, which
TCG can't exercise.

== Tree / config ==

  base:  v7.1-rc7
  clang: 22.1.7 (LLVM=-22; needed for the context-analysis check)
  ARM multi_v7_defconfig + CONFIG_CC_IS_CLANG, WARN_CONTEXT_ANALYSIS,
  SMP, ASPEED_LPC_SNOOP, HARDENED_USERCOPY, PROVE_LOCKING,
  DEBUG_ATOMIC_SLEEP.

  make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=-22 O=build \
       -j$(nproc) zImage

== Run ==

The injection module (full source below the sign-off) creates two
write-only sysfs knobs under /sys/kernel/snoop_test/. The init loads it,
then:

  echo 4096 > /sys/kernel/snoop_test/generate    # fill via the real
                                                  # IRQ path 
  echo 1    > /sys/kernel/snoop_test/adjust_ptrs  # force in=4097/out=1
                                                  
  read(fd, buf, 4096) from /dev/aspeed-lpc-snoop0 # the overflowing read

Build it against the same tree (-DSNOOP_PATCHED for v6, which mirrors the
spinlock the fix adds ahead of @fifo; omit it for the unpatched build).

  qemu-system-arm -M ast2600-evb -smp 2 \
    -kernel build/arch/arm/boot/zImage \
    -dtb build/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb \
    -initrd repro.cpio.gz \
    -append "console=ttyS4,115200 panic=-1" -nographic -no-reboot

== Result ==

Unpatched, read(4096) with in=4097/out=1:

  usercopy: Kernel memory exposure attempt detected from SLUB object
  'kmalloc-2k' (offset 0, size 2049)!
   kfifo_copy_to_user / __kfifo_to_user / snoop_file_read / vfs_read
  Kernel panic - not syncing: Fatal exception

Patched: read() returns 2048, no panic; no lockdep or atomic-sleep
splats.

The init is just: mount proc/sysfs/devtmpfs, finit_module() the .ko,
write the two knobs above, then read(4096) from the char device. Full
injection module follows.

Thanks,
Karthikeyan

------ snoop_test_inject.c (build as an out-of-tree module) ------

// SPDX-License-Identifier: GPL-2.0
/*
 * Reproduce the aspeed-lpc-snoop kfifo SPSC-violation post-race state
 * deterministically under QEMU. Two write-only sysfs knobs under
 * /sys/kernel/snoop_test/:
 *
 *   generate <count>  Push <count> POST-code bytes through the *real*
 *                     driver IRQ path: write SNPWDR + HICR6 via the LPC
 *                     syscon regmap, then dispatch the snoop IRQ so
 *                     aspeed_lpc_snoop_irq() -> put_fifo_with_discard()
 *                     runs.
 *
 *   adjust_ptrs <1>   Force the channel-0 kfifo into the state a reader
 *                     observes inside the race window: in = 4097,
 *                     out = 1, so (in - out) = 4096 > the 2048-byte ring.
 *
 * The driver's private channel is reached through a mirror struct whose
 * layout must match drivers/soc/aspeed/aspeed-lpc-snoop.c. The v6 fix
 * inserts a spinlock_t ahead of @fifo -- build with -DSNOOP_PATCHED to
 * mirror that, otherwise the &fifo offset is wrong.
 */
#include <linux/module.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/version.h>
#include <linux/fs.h>
#include <linux/file.h>
#include <linux/kfifo.h>
#include <linux/miscdevice.h>
#include <linux/regmap.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdesc.h>
#include <linux/device.h>
#include <linux/kobject.h>
#include <linux/sysfs.h>
#include <linux/wait.h>
#include <linux/spinlock.h>
#include <linux/bitops.h>

#define HICR6			0x84
#define HICR6_STR_SNP0W		BIT(0)
#define SNPWDR			0x94
#define SNOOP_DEV		"/dev/aspeed-lpc-snoop0"
#define RACE_OUT		1u
#define RACE_IN			4097u

struct snoop_chan_mirror {
	const void		*cfg;
	bool			enabled;
#ifdef SNOOP_PATCHED
	spinlock_t		lock;		/* added by the fix */
#endif
	struct kfifo		fifo;
	wait_queue_head_t	wq;
	struct miscdevice	miscdev;
};

struct snoop_dev_mirror {
	struct regmap		*regmap;
	int			irq;
	/* struct clk *clk; struct aspeed_lpc_snoop_channel chan[2]; follow */
};

static struct file *snoop_open(struct snoop_chan_mirror **chan_out)
{
	struct file *filp;
	struct miscdevice *md;

	filp = filp_open(SNOOP_DEV, O_RDONLY | O_NONBLOCK, 0);
	if (IS_ERR(filp))
		return filp;

	md = filp->private_data;
	if (!md) {
		filp_close(filp, NULL);
		return ERR_PTR(-ENODEV);
	}

	*chan_out = container_of(md, struct snoop_chan_mirror, miscdev);
	return filp;
}

static ssize_t generate_store(struct kobject *kobj, struct kobj_attribute *attr,
			      const char *buf, size_t len)
{
	struct snoop_chan_mirror *chan;
	struct snoop_dev_mirror *sdev;
	struct file *filp;
	unsigned int count, i;
	int rc;

	rc = kstrtouint(buf, 0, &count);
	if (rc)
		return rc;

	filp = snoop_open(&chan);
	if (IS_ERR(filp))
		return PTR_ERR(filp);

	sdev = dev_get_drvdata(chan->miscdev.parent);
	if (!sdev || !sdev->regmap || sdev->irq <= 0) {
		filp_close(filp, NULL);
		return -ENODEV;
	}

	for (i = 0; i < count; i++) {
		/* Stage the snoop'ed byte and the data-ready status bit. */
		regmap_write(sdev->regmap, SNPWDR, (u32)(i & 0xff));
		regmap_write(sdev->regmap, HICR6, HICR6_STR_SNP0W);

		/*
		 * Dispatch IRQ -> aspeed_lpc_snoop_irq() ->
		 * put_fifo_with_discard(). generic_handle_irq_safe() copes
		 * with the GIC requiring the handler to run with IRQs off.
		 */
		generic_handle_irq_safe(sdev->irq);
	}

	filp_close(filp, NULL);
	return len;
}

static ssize_t adjust_ptrs_store(struct kobject *kobj, struct kobj_attribute *attr,
				 const char *buf, size_t len)
{
	struct snoop_chan_mirror *chan;
	struct file *filp;
	struct __kfifo *kf;
	unsigned int val;
	int rc;

	rc = kstrtouint(buf, 0, &val);
	if (rc)
		return rc;
	if (val != 1)
		return -EINVAL;

	filp = snoop_open(&chan);
	if (IS_ERR(filp))
		return PTR_ERR(filp);

	kf = &chan->fifo.kfifo;
	/* Reproduce the race outcome: fresh 'in', stale 'out'. */
	WRITE_ONCE(kf->out, RACE_OUT);
	WRITE_ONCE(kf->in, RACE_IN);
	pr_info("snoop_test: in=%u out=%u (in-out=%u, size=%u)\n",
		kf->in, kf->out, kf->in - kf->out, kf->mask + 1);

	filp_close(filp, NULL);
	return len;
}

static struct kobj_attribute generate_attr =
	__ATTR(generate, 0220, NULL, generate_store);
static struct kobj_attribute adjust_ptrs_attr =
	__ATTR(adjust_ptrs, 0220, NULL, adjust_ptrs_store);

static struct attribute *snoop_attrs[] = {
	&generate_attr.attr,
	&adjust_ptrs_attr.attr,
	NULL,
};
static const struct attribute_group snoop_group = { .attrs = snoop_attrs };
static struct kobject *snoop_kobj;

static int __init snoop_test_init(void)
{
	int rc;

	snoop_kobj = kobject_create_and_add("snoop_test", kernel_kobj);
	if (!snoop_kobj)
		return -ENOMEM;

	rc = sysfs_create_group(snoop_kobj, &snoop_group);
	if (rc) {
		kobject_put(snoop_kobj);
		return rc;
	}
	return 0;
}

static void __exit snoop_test_exit(void)
{
	sysfs_remove_group(snoop_kobj, &snoop_group);
	kobject_put(snoop_kobj);
}

module_init(snoop_test_init);
module_exit(snoop_test_exit);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("aspeed-lpc-snoop kfifo race post-state reproducer");

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

On Tue, 2026-06-16 at 07:30 +0000, Karthikeyan KS wrote:
> Hi Andrew,
> 
> Happy to. Short version: ast2600-evb can't hit the SMP timing window,
> so I reproduce each missing piece deliberately. The driver code under
> test is unmodified -- only the stimulus and the post-race state are
> injected. Stock qemu-system-arm (Debian 8.2.2), no QEMU changes.
> 
> Three obstacles, and what I did about each:

This looks like a lot of heavily LLM-assisted effort. Please review the
relevant documentation, starting here:

   https://docs.kernel.org/process/submitting-patches.html#using-assisted-by

I feel the testing strategy is pretty questionable. Any invariant
violation is possible with that type of meddling.

I was interested in whether you drove the interrupt sequence via
emulated hardware. I asked because upstream qemu doesn't currently
support the snoop device.

In v3 you said:

   The issue was observed on physical AST2600 (dual-core Cortex-A7)
   in production under heavy POST code traffic during concurrent
   userspace reads.

   https://lore.kernel.org/all/20260527175939.2939714-1-karthiproffesional@gmail.com/

Is this true? What platform did you test with?

Andrew

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

This looks like a lot of heavily LLM-assisted effort. Please review the
relevant documentation, starting here:
   https://docs.kernel.org/process/submitting-patches.html#using-assisted-by

==> I partly agree. The code and bug analysis are done manually.
LLM use was the out of tree test harness and lightly polishing
the commit message. None of the submitted code is generated.
If you'd prefer, I can reword the changelog in my own words or
add an Assisted-by tag ?

V1         was a simple clamp
v2/V3      was a simple locking mechanism which is pretty straight forward.
V4         bounce buffer, modelled on gpiolib-cdev (acknowledged in patch)
V5 and V6  entirely your review comments (__free, scoped_guard,
           kfifo_out_spinlocked, __guarded_by, context analysis)


I feel the testing strategy is pretty questionable. Any invariant
violation is possible with that type of meddling.

==> The underlying bug is a kfifo SPSC contract violation. My intent with the
test wasn't to simulate the race itself, but to reconstruct the post race state
specifically where (in - out) exceeds the buffer size and show it causes a
usercopy overflow in the unpatched code, handled safely after the fix.

==> I take your point that forcing that state can itself produce violations that
wouldn't occur naturally. Since the bug is provable from the source but hard to
trigger on demand, I'd rather ask what validation you'd accept here?

I was interested in whether you drove the interrupt sequence via
emulated hardware. I asked because upstream qemu doesn't currently
support the snoop device.

==> My apologies for the confusion, I mixed up things. I have not driven the
interrupt sequence in emulation; as you noted, upstream QEMU doesn't model the
snoop device. I've described the actual hardware context below.

In v3 you said:
   The issue was observed on physical AST2600 (dual-core Cortex-A7)
   in production under heavy POST code traffic during concurrent
   userspace reads.
   https://lore.kernel.org/all/20260527175939.2939714-1-karthiproffesional@gmail.com/
Is this true? What platform did you test with?

==> Yes, the underlying failure is real. It was observed on an AST2600-based
production BMC running a vendor BSP kernel under continuous host reboot
cycles. Because that platform can't currently be brought up on pure
mainline without substantial out-of-tree board support, I have not run
this exact mainline patch on the physical silicon, observed under the
BSP kernel, not yet verified as the mainline patch. I should have made
that distinction clear earlier in the thread.

==> If there's a way you'd consider valid for validating a fix like this
without a full mainline bring up on the SoC, such as a targeted kfifo unit
test, or something else you'd accept.I'd appreciate the pointer and I'll
do that.

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

On Wed, 2026-06-17 at 13:10 +0000, Karthikeyan KS wrote:
> This looks like a lot of heavily LLM-assisted effort. Please review the
> relevant documentation, starting here:
>    https://docs.kernel.org/process/submitting-patches.html#using-assisted-by
> 
> ==> I partly agree. The code and bug analysis are done manually.
> LLM use was the out of tree test harness and lightly polishing
> the commit message. None of the submitted code is generated.
> If you'd prefer, I can reword the changelog in my own words or
> add an Assisted-by tag ?
> 

Thanks for the clarification. It's probably okay as-is in that case,
but that was unclear previously.

> I feel the testing strategy is pretty questionable. Any invariant
> violation is possible with that type of meddling.
> 
> ==> The underlying bug is a kfifo SPSC contract violation. My intent with the
> test wasn't to simulate the race itself, but to reconstruct the post race state
> specifically where (in - out) exceeds the buffer size and show it causes a
> usercopy overflow in the unpatched code, handled safely after the fix.
> 
> ==> I take your point that forcing that state can itself produce violations that
> wouldn't occur naturally. Since the bug is provable from the source but hard to
> trigger on demand, I'd rather ask what validation you'd accept here?

I'm aiming to build confidence that the change has been tested in
practice beyond spherical-cow circumstances. Isolating the conditions
this way seems okay, but I'd class the testing approach as necessary-
but-not-sufficient. It's important that the change is tested under
typical conditions to build confidence against regressions, as well as
atypical conditions.

> 
> I was interested in whether you drove the interrupt sequence via
> emulated hardware. I asked because upstream qemu doesn't currently
> support the snoop device.
> 
> ==> My apologies for the confusion, I mixed up things. I have not driven the
> interrupt sequence in emulation; as you noted, upstream QEMU doesn't model the
> snoop device. I've described the actual hardware context below.
> 
> In v3 you said:
>    The issue was observed on physical AST2600 (dual-core Cortex-A7)
>    in production under heavy POST code traffic during concurrent
>    userspace reads.
>    https://lore.kernel.org/all/20260527175939.2939714-1-karthiproffesional@gmail.com/
> Is this true? What platform did you test with?
> 
> ==> Yes, the underlying failure is real. It was observed on an AST2600-based
> production BMC running a vendor BSP kernel under continuous host reboot
> cycles. Because that platform can't currently be brought up on pure
> mainline without substantial out-of-tree board support, I have not run
> this exact mainline patch on the physical silicon, observed under the
> BSP kernel, not yet verified as the mainline patch. I should have made
> that distinction clear earlier in the thread.

Can you confirm you you have tested on hardware a backport of this
patch to your BSP kernel?

> 
> ==> If there's a way you'd consider valid for validating a fix like this
> without a full mainline bring up on the SoC, such as a targeted kfifo unit
> test, or something else you'd accept.I'd appreciate the pointer and I'll
> do that.

No, I believe the change is fine, but the claim of testing under qemu
when qemu doesn't model the necessary hardware was a red flag that
needed to be addressed, doubly so in the absence of your track record
of upstream work.

Thanks,

Andrew

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

On Wed, 2026-06-17 at 13:10 +0000, Andrew Jeffery wrote:
> Can you confirm you have tested on hardware a backport of this
> patch to your BSP kernel?

Not until yesterday, done now. Backported the fix to
the BSP kernel (5.4.x) on the AST2600 BMC where the original failure
was observed. Tested under continuous host reboot cycles with concurrent
userspace reads on /dev/aspeed-lpc-snoop0. no panic, no usercopy
splat. Same workload on the unpatched BSP reproduced reliably.

Thanks,
Karthikeyan

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

Hi Andrew,

Just following up, it's been about two weeks since I shared the
hardware validation results. I believe I've addressed the questions
raised in the thread, but please let me know if there's anything
else you'd like me to do.

Thanks,
Karthikeyan

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Andrew Jeffery]

Hi,

On Wed, 2026-07-01 at 17:15 +0000, Karthikeyan KS wrote:
> Hi Andrew,
> 
> Just following up, it's been about two weeks since I shared the
> hardware validation results. I believe I've addressed the questions
> raised in the thread, but please let me know if there's anything
> else you'd like me to do.

Nothing further required on your part. I intend to apply it, however am
working through some other priorities.

Thanks,

Andrew

Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[Karthikeyan KS]

Hi Andrew,

Thanks for your consistent support and guidance.
I learned a lot from you.
Much Appreciated.

Thanks,
Karthikeyan

[PATCH] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

[root]

From: Karthikeyan KS <karthiproffesional@gmail.com>

snoop_file_read() passes the userspace count directly to
kfifo_to_user() without clamping. The kfifo backing buffer is
2048 bytes (SNOOP_FIFO_SIZE), allocated from kmalloc-2k slab.
A read larger than 2048 bytes triggers a BUG under
CONFIG_HARDENED_USERCOPY:

  kernel BUG at mm/usercopy.c:99!

Reproducer:
  hexdump /dev/aspeed-lpc-snoop0

Fix by clamping count to SNOOP_FIFO_SIZE before the copy.

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Cc: stable@vger.kernel.org
Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index b03310c0830d..5b59e826cc68 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -125,6 +125,7 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
        if (ret == -ERESTARTSYS)
            return -EINTR;
    }
+   count = min(count, (size_t)SNOOP_FIFO_SIZE);
    ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
    if (ret)
        return ret;
-- 
2.34.1