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

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.

Karthikeyan

On Thu, Jun 11, 2026 at 8:20 AM Andrew Jeffery <[email protected]>
wrote:

> 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;
>