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

[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:99!
  Call trace:
   usercopy_abort
   __check_heap_object
   __check_object_size
   kfifo_copy_to_user
   __kfifo_to_user
   snoop_file_read
   vfs_read

Reproduced on ast2600-evb (dual-core ARM Cortex-A7) when the host floods
POST codes while userspace reads /dev/aspeed-lpc-snoop0.

Serialize kfifo access with a per-channel spinlock: use spin_lock()/
spin_unlock() in put_fifo_with_discard() (hardirq only) and
spin_lock_irq()/spin_unlock_irq() around kfifo_to_user() in
snoop_file_read().

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.

> The AST2500 has a (single-core) ARM1176JZS

Corrected in v3.

> Don't double-account for the bug

Agreed — the spinlock eliminates the unsynchronized window that
produces the inconsistent pointer state. Clamp removed.

> _irqsave isn't wrong

Changed to spin_lock_irq — fops callbacks always enter with
interrupts enabled.

> Can you provide more details? The 2500 is single-core

The issue was observed on physical AST2600 (dual-core Cortex-A7)
in production under heavy POST code traffic during concurrent
userspace reads. Since the x86 host does not model ARM weak memory
ordering, the race cannot be reproduced naturally in QEMU. The test
module adjusts kfifo pointers to reproduce the post-race state for
deterministic validation.

> AST2600 has a dual-core Cortex-A7, so your bug makes more sense there

Yes, the issue is intermittently observed on production AST2600.

Changes since v2:
- Dropped count clamp
- spin_lock_irqsave -> spin_lock_irq in snoop_file_read
- Fixed platform: AST2600 (dual-core Cortex-A7)
- Trimmed backtrace
- Added Fixes tag

 drivers/soc/aspeed/aspeed-lpc-snoop.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index eceeaf8df..ef6697a42 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -60,6 +60,7 @@ struct aspeed_lpc_snoop_model_data {
 
 struct aspeed_lpc_snoop_channel {
 	struct kfifo		fifo;
+	spinlock_t		lock;
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
 };
@@ -93,7 +94,11 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,
 		if (ret == -ERESTARTSYS)
 			return -EINTR;
 	}
+
+	spin_lock_irq(&chan->lock);
 	ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);
+	spin_unlock_irq(&chan->lock);
+
 	if (ret)
 		return ret;
 
@@ -121,9 +126,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);
 	wake_up_interruptible(&chan->wq);
 }
 
@@ -192,6 +199,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		of_device_get_match_data(dev);
 
 	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
+	spin_lock_init(&lpc_snoop->chan[channel].lock);
 	/* Create FIFO datastructure */
 	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
 			 SNOOP_FIFO_SIZE, GFP_KERNEL);
-- 
2.43.0