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

[Andrew Jeffery <andrew@codeconstruct.com.au>]

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;