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@xxxxxxxxxxxxxxx
> Signed-off-by: Karthikeyan KS <karthiproffesional@xxxxxxxxx>
> ---
> 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;