* [PATCH v5] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
@ 2026-06-10 17:23 ` Karthikeyan KS
2026-06-11 2:50 ` Andrew Jeffery
2026-06-11 18:08 ` Karthikeyan KS
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Karthikeyan KS @ 2026-06-10 17:23 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening, Karthikeyan KS, stable
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 17:23 ` [PATCH v5] " Karthikeyan KS
@ 2026-06-11 2:50 ` Andrew Jeffery
[not found] ` <CAP_JKPu9MTpMUZmg9BY3sxGhmBzgR0E6HnvAT7sQjVUpQp0dSQ@mail.gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Jeffery @ 2026-06-11 2:50 UTC (permalink / raw)
To: Karthikeyan KS
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening, stable
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;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
2026-06-10 17:23 ` [PATCH v5] " Karthikeyan KS
@ 2026-06-11 18:08 ` Karthikeyan KS
2026-06-12 19:07 ` [PATCH v6] " Karthikeyan KS
` (5 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Karthikeyan KS @ 2026-06-11 18:08 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
2026-06-10 17:23 ` [PATCH v5] " Karthikeyan KS
2026-06-11 18:08 ` Karthikeyan KS
@ 2026-06-12 19:07 ` Karthikeyan KS
2026-06-16 0:20 ` Andrew Jeffery
2026-06-16 7:30 ` Karthikeyan KS
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Karthikeyan KS @ 2026-06-12 19:07 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-12 19:07 ` [PATCH v6] " Karthikeyan KS
@ 2026-06-16 0:20 ` Andrew Jeffery
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jeffery @ 2026-06-16 0:20 UTC (permalink / raw)
To: Karthikeyan KS
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
` (2 preceding siblings ...)
2026-06-12 19:07 ` [PATCH v6] " Karthikeyan KS
@ 2026-06-16 7:30 ` Karthikeyan KS
2026-06-17 0:44 ` Andrew Jeffery
2026-06-17 13:10 ` Karthikeyan KS
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Karthikeyan KS @ 2026-06-16 7:30 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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");
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-16 7:30 ` Karthikeyan KS
@ 2026-06-17 0:44 ` Andrew Jeffery
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jeffery @ 2026-06-17 0:44 UTC (permalink / raw)
To: Karthikeyan KS
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
` (3 preceding siblings ...)
2026-06-16 7:30 ` Karthikeyan KS
@ 2026-06-17 13:10 ` Karthikeyan KS
2026-06-18 0:44 ` Andrew Jeffery
2026-06-20 13:11 ` Karthikeyan KS
` (2 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Karthikeyan KS @ 2026-06-17 13:10 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-17 13:10 ` Karthikeyan KS
@ 2026-06-18 0:44 ` Andrew Jeffery
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jeffery @ 2026-06-18 0:44 UTC (permalink / raw)
To: Karthikeyan KS
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
` (4 preceding siblings ...)
2026-06-17 13:10 ` Karthikeyan KS
@ 2026-06-20 13:11 ` Karthikeyan KS
2026-07-01 17:15 ` Karthikeyan KS
2026-07-02 17:35 ` Karthikeyan KS
7 siblings, 0 replies; 21+ messages in thread
From: Karthikeyan KS @ 2026-06-20 13:11 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
` (5 preceding siblings ...)
2026-06-20 13:11 ` Karthikeyan KS
@ 2026-07-01 17:15 ` Karthikeyan KS
2026-07-02 0:25 ` Andrew Jeffery
2026-07-02 17:35 ` Karthikeyan KS
7 siblings, 1 reply; 21+ messages in thread
From: Karthikeyan KS @ 2026-07-01 17:15 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-07-01 17:15 ` Karthikeyan KS
@ 2026-07-02 0:25 ` Andrew Jeffery
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jeffery @ 2026-07-02 0:25 UTC (permalink / raw)
To: Karthikeyan KS
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6] soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read
2026-06-10 2:26 ` Andrew Jeffery
` (6 preceding siblings ...)
2026-07-01 17:15 ` Karthikeyan KS
@ 2026-07-02 17:35 ` Karthikeyan KS
7 siblings, 0 replies; 21+ messages in thread
From: Karthikeyan KS @ 2026-07-02 17:35 UTC (permalink / raw)
To: andrew
Cc: joel, andrew, Kees Cook, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-hardening
Hi Andrew,
Thanks for your consistent support and guidance.
I learned a lot from you.
Much Appreciated.
Thanks,
Karthikeyan
^ permalink raw reply [flat|nested] 21+ messages in thread