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: [email protected]
> Signed-off-by: Karthikeyan KS <[email protected]>
> ---
> 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;
