soc: aspeed: lpc-snoop: Fix usercopy overflow in snoop_file_read

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>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d745929646fb976c9902ff03de71c02b9906f..b5188dcde37a71ed6ac06a02aa2e7f6aa10fe2c0 100644 (file)
--- 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 28e491fffc7d71b36545d2d98bb8707ebe343c96..b09dca34099fe54b2dff7de5299201401966cfa9 100644 (file)
--- 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)