drivers/soc/aspeed/Makefile | 1 + drivers/soc/aspeed/aspeed-lpc-snoop.c | 38 ++++++++++++++++++--------- 2 files changed, 27 insertions(+), 12 deletions(-)
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 shared between the
IRQ handler (producer) and the file reader (consumer). Annotate @fifo
with __guarded_by(&lock) and opt the driver into context analysis so the
compiler enforces that all fifo access holds the lock.
Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Signed-off-by: Karthikeyan KS <karthiproffesional@gmail.com>
---
drivers/soc/aspeed/Makefile | 1 +
drivers/soc/aspeed/aspeed-lpc-snoop.c | 38 ++++++++++++++++++---------
2 files changed, 27 insertions(+), 12 deletions(-)
Andrew,
Thanks for the review.
Changes since v5:
- Annotate @fifo with __guarded_by(&lock) instead of a comment
- Move kfifo_initialized() check inside scoped_guard(spinlock, &chan->lock)
in put_fifo_with_discard()
- Replace spin_lock_init() with scoped_guard(spinlock_init, &channel->lock)
around kfifo_alloc() in aspeed_lpc_enable_snoop()
- Enable CONTEXT_ANALYSIS for this driver in drivers/soc/aspeed/Makefile
Dropped Cc: stable — the fix uses cleanup.h/context-analysis idioms absent
from LTS; I'll send adapted backports to stable@ once this is in mainline.
Tested on ast2600-evb (QEMU): clang-22 with CONFIG_WARN_CONTEXT_ANALYSIS=y
shows no context-analysis warnings; PROVE_LOCKING, DEBUG_ATOMIC_SLEEP and
HARDENED_USERCOPY show no splats. Overflow reproduced via a fault-injection
module forcing the post-race (in - out) state (QEMU doesn't model the ARM
ordering that triggers it in the field): unpatched panics, patched returns
cleanly.
Thanks,
Karthikeyan
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..b5188dcde37a 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -4,3 +4,4 @@ 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 b03310c0830d..7fa1a345acac 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,7 +75,8 @@ struct aspeed_lpc_snoop_channel_cfg {
struct aspeed_lpc_snoop_channel {
const struct aspeed_lpc_snoop_channel_cfg *cfg;
bool enabled;
- struct kfifo fifo;
+ spinlock_t lock;
+ struct kfifo fifo __guarded_by(&lock);
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;
}
@@ -151,11 +161,13 @@ 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;
- if (kfifo_is_full(&chan->fifo))
- kfifo_skip(&chan->fifo);
- kfifo_put(&chan->fifo, val);
+ 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);
+ }
wake_up_interruptible(&chan->wq);
}
@@ -239,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)
--
2.43.0
© 2016 - 2026 Red Hat, Inc.