Re: [PATCH v2] USB: cdc-wdm: fix read buffer overflow

[Oliver Neukum]

On Thursday 14 February 2013 18:10:43 Bjørn Mork wrote:
> Do not scribble past end of buffer.  Check if the userspace buffer has
> enough space available before attempting to move more data there. Drop
> new data on overflow.
> 
> Cc: stable <sta...@vger.kernel.org>
> Signed-off-by: Bjørn Mork <bj...@mork.no>
> ---
> Oliver Neukum <oneu...@suse.de> writes:
> 
> > I am afraid your diagnosis is correct. This is a buffer overflow. Not good.
> > The fix is a problem. It seems to me that we need to throw away the
> > newest data and report an error.
> 
> OK.  I preferred receiving the newest data for my use case, but I trust that
> you know what's best as usual :-)

We have to let user space recover. To do so we need to indicate when
exactly we dropped data.
How about this?

        Regards
                Oliver

>From 6be64bd7522f1c11a535741840797030c0436acf Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneu...@suse.de>
Date: Thu, 14 Feb 2013 21:26:35 +0100
Subject: [PATCH] cdc-wdm: fix buffer overflow

If a read overflows the limit under which a single transfer
can overflow the buffer we allow no further data into the buffer
and remember an error

Signed-off-by: Oliver Neukum <oneu...@suse.de>
---
 drivers/usb/class/cdc-wdm.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 5f0cb41..2a5963b 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -184,10 +184,19 @@ static void wdm_in_callback(struct urb *urb)
                }
        }
 
+       /* hopeless congestion or error*/
+       if (desc->rerr < 0)
+               goto skip_error;
+
        desc->rerr = status;
        desc->reslength = urb->actual_length;
+       if (desc->length > desc->wMaxCommand) {
+               /* the buffer is full */
+               desc->rerr = -ENOBUFS;
+       }
        memmove(desc->ubuf + desc->length, desc->inbuf, desc->reslength);
        desc->length += desc->reslength;
+
 skip_error:
        wake_up(&desc->wait);
 
@@ -418,7 +427,7 @@ outnl:
 static ssize_t wdm_read
 (struct file *file, char __user *buffer, size_t count, loff_t *ppos)
 {
-       int rv, cntr;
+       int rv, cntr, err;
        int i = 0;
        struct wdm_device *desc = file->private_data;
 
@@ -465,10 +474,13 @@ retry:
                spin_lock_irq(&desc->iuspin);
 
                if (desc->rerr) { /* read completed, error happened */
-                       desc->rerr = 0;
-                       spin_unlock_irq(&desc->iuspin);
-                       rv = -EIO;
-                       goto err;
+                       if (!desc->reslength) {
+                               err = desc->rerr;
+                               desc->rerr = 0;
+                               spin_unlock_irq(&desc->iuspin);
+                               rv = (err == -ENOBUFS) ? err : -EIO;
+                               goto err;
+                       }
                }
                /*
                 * recheck whether we've lost the race
@@ -714,7 +726,7 @@ static int wdm_create(struct usb_interface *intf, struct 
usb_endpoint_descriptor
        if (!desc->command)
                goto err;
 
-       desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
+       desc->ubuf = kmalloc(desc->wMaxCommand * 2, GFP_KERNEL);
        if (!desc->ubuf)
                goto err;
 
-- 
1.7.10.4


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html