| Message ID | 1366110979-15016-1-git-send-email-luis.henriques@canonical.com |
|---|---|
| State | New |
| Headers | show |
Look to be following the upstream change very closely.
On Tue, Apr 16, 2013 at 12:16:19PM +0100, Luis Henriques wrote: > From: Oliver Neukum <oneukum@suse.de> > > CVE-2013-1860 > > BugLink: http://bugs.launchpad.net/bugs/1156784 > > The buffer for responses must not overflow. > If this would happen, set a flag, drop the data and return > an error after user space has read all remaining data. > > Signed-off-by: Oliver Neukum <oliver@neukum.org> > CC: stable@kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (back ported from commit c0f5ecee4e741667b2493c742b60b6218d40b3aa) > > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > drivers/usb/class/cdc-wdm.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c > index 37f2899..6ca1363 100644 > --- a/drivers/usb/class/cdc-wdm.c > +++ b/drivers/usb/class/cdc-wdm.c > @@ -52,7 +52,7 @@ MODULE_DEVICE_TABLE (usb, wdm_ids); > #define WDM_READ 4 > #define WDM_INT_STALL 5 > #define WDM_POLL_RUNNING 6 > - > +#define WDM_OVERFLOW 10 Odd, but follows upstream allocation. > #define WDM_MAX 16 > > @@ -115,6 +115,7 @@ static void wdm_in_callback(struct urb *urb) > { > struct wdm_device *desc = urb->context; > int status = urb->status; > + int length = urb->actual_length; > > spin_lock(&desc->iuspin); > > @@ -144,9 +145,17 @@ static void wdm_in_callback(struct urb *urb) > } > > desc->rerr = status; > - desc->reslength = urb->actual_length; > - memmove(desc->ubuf + desc->length, desc->inbuf, desc->reslength); > - desc->length += desc->reslength; > + if (length + desc->length > desc->wMaxCommand) { > + /* The buffer would overflow */ > + set_bit(WDM_OVERFLOW, &desc->flags); > + } else { > + /* we may already be in overflow */ > + if (!test_bit(WDM_OVERFLOW, &desc->flags)) { > + memmove(desc->ubuf + desc->length, desc->inbuf, length); > + desc->length += length; > + desc->reslength = length; > + } > + } Why this is not } else if (!test_bit(...)) { I do not know, but it follows the upstream commit. > wake_up(&desc->wait); > > set_bit(WDM_READ, &desc->flags); > @@ -398,6 +407,11 @@ retry: > rv = -ENODEV; > goto err; > } > + if (test_bit(WDM_OVERFLOW, &desc->flags)) { > + clear_bit(WDM_OVERFLOW, &desc->flags); > + rv = -ENOBUFS; > + goto err; > + } > i++; > if (file->f_flags & O_NONBLOCK) { > if (!test_bit(WDM_READ, &desc->flags)) { > @@ -440,6 +454,7 @@ retry: > spin_unlock_irq(&desc->iuspin); > goto retry; > } > + > if (!desc->reslength) { /* zero length read */ > dev_dbg(&desc->intf->dev, "%s: zero length - clearing WDM_READ\n", __func__); > clear_bit(WDM_READ, &desc->flags); > @@ -844,6 +859,7 @@ static int wdm_post_reset(struct usb_interface *intf) > struct wdm_device *desc = usb_get_intfdata(intf); > int rv; > > + clear_bit(WDM_OVERFLOW, &desc->flags); > rv = recover_from_urb_loss(desc); > mutex_unlock(&desc->plock); > return 0; Looks to carry the essentials of the upstream commit to my eye. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
On Tue, Apr 16, 2013 at 01:40:44PM +0100, Andy Whitcroft wrote: > On Tue, Apr 16, 2013 at 12:16:19PM +0100, Luis Henriques wrote: > > From: Oliver Neukum <oneukum@suse.de> > > > > CVE-2013-1860 > > > > BugLink: http://bugs.launchpad.net/bugs/1156784 > > > > The buffer for responses must not overflow. > > If this would happen, set a flag, drop the data and return > > an error after user space has read all remaining data. > > > > Signed-off-by: Oliver Neukum <oliver@neukum.org> > > CC: stable@kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > (back ported from commit c0f5ecee4e741667b2493c742b60b6218d40b3aa) > > > > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > > --- > > drivers/usb/class/cdc-wdm.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c > > index 37f2899..6ca1363 100644 > > --- a/drivers/usb/class/cdc-wdm.c > > +++ b/drivers/usb/class/cdc-wdm.c > > @@ -52,7 +52,7 @@ MODULE_DEVICE_TABLE (usb, wdm_ids); > > #define WDM_READ 4 > > #define WDM_INT_STALL 5 > > #define WDM_POLL_RUNNING 6 > > - > > +#define WDM_OVERFLOW 10 > > Odd, but follows upstream allocation. Well, it really doesn't make a difference. I just though it would be better to track upstream. This way we can eventually add other definitions in between and keep using the same values. > > > #define WDM_MAX 16 > > > > @@ -115,6 +115,7 @@ static void wdm_in_callback(struct urb *urb) > > { > > struct wdm_device *desc = urb->context; > > int status = urb->status; > > + int length = urb->actual_length; > > > > spin_lock(&desc->iuspin); > > > > @@ -144,9 +145,17 @@ static void wdm_in_callback(struct urb *urb) > > } > > > > desc->rerr = status; > > - desc->reslength = urb->actual_length; > > - memmove(desc->ubuf + desc->length, desc->inbuf, desc->reslength); > > - desc->length += desc->reslength; > > + if (length + desc->length > desc->wMaxCommand) { > > + /* The buffer would overflow */ > > + set_bit(WDM_OVERFLOW, &desc->flags); > > + } else { > > + /* we may already be in overflow */ > > + if (!test_bit(WDM_OVERFLOW, &desc->flags)) { > > + memmove(desc->ubuf + desc->length, desc->inbuf, length); > > + desc->length += length; > > + desc->reslength = length; > > + } > > + } > > Why this is not } else if (!test_bit(...)) { I do not know, but it > follows the upstream commit. Heh, I found that amusing as well. > > > wake_up(&desc->wait); > > > > set_bit(WDM_READ, &desc->flags); > > @@ -398,6 +407,11 @@ retry: > > rv = -ENODEV; > > goto err; > > } > > + if (test_bit(WDM_OVERFLOW, &desc->flags)) { > > + clear_bit(WDM_OVERFLOW, &desc->flags); > > + rv = -ENOBUFS; > > + goto err; > > + } > > i++; > > if (file->f_flags & O_NONBLOCK) { > > if (!test_bit(WDM_READ, &desc->flags)) { > > @@ -440,6 +454,7 @@ retry: > > spin_unlock_irq(&desc->iuspin); > > goto retry; > > } > > + > > if (!desc->reslength) { /* zero length read */ > > dev_dbg(&desc->intf->dev, "%s: zero length - clearing WDM_READ\n", __func__); > > clear_bit(WDM_READ, &desc->flags); > > @@ -844,6 +859,7 @@ static int wdm_post_reset(struct usb_interface *intf) > > struct wdm_device *desc = usb_get_intfdata(intf); > > int rv; > > > > + clear_bit(WDM_OVERFLOW, &desc->flags); > > rv = recover_from_urb_loss(desc); > > mutex_unlock(&desc->plock); > > return 0; > > Looks to carry the essentials of the upstream commit to my eye. > > Acked-by: Andy Whitcroft <apw@canonical.com> > > -apw Cheers, -- Luis
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index 37f2899..6ca1363 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -52,7 +52,7 @@ MODULE_DEVICE_TABLE (usb, wdm_ids); #define WDM_READ 4 #define WDM_INT_STALL 5 #define WDM_POLL_RUNNING 6 - +#define WDM_OVERFLOW 10 #define WDM_MAX 16 @@ -115,6 +115,7 @@ static void wdm_in_callback(struct urb *urb) { struct wdm_device *desc = urb->context; int status = urb->status; + int length = urb->actual_length; spin_lock(&desc->iuspin); @@ -144,9 +145,17 @@ static void wdm_in_callback(struct urb *urb) } desc->rerr = status; - desc->reslength = urb->actual_length; - memmove(desc->ubuf + desc->length, desc->inbuf, desc->reslength); - desc->length += desc->reslength; + if (length + desc->length > desc->wMaxCommand) { + /* The buffer would overflow */ + set_bit(WDM_OVERFLOW, &desc->flags); + } else { + /* we may already be in overflow */ + if (!test_bit(WDM_OVERFLOW, &desc->flags)) { + memmove(desc->ubuf + desc->length, desc->inbuf, length); + desc->length += length; + desc->reslength = length; + } + } wake_up(&desc->wait); set_bit(WDM_READ, &desc->flags); @@ -398,6 +407,11 @@ retry: rv = -ENODEV; goto err; } + if (test_bit(WDM_OVERFLOW, &desc->flags)) { + clear_bit(WDM_OVERFLOW, &desc->flags); + rv = -ENOBUFS; + goto err; + } i++; if (file->f_flags & O_NONBLOCK) { if (!test_bit(WDM_READ, &desc->flags)) { @@ -440,6 +454,7 @@ retry: spin_unlock_irq(&desc->iuspin); goto retry; } + if (!desc->reslength) { /* zero length read */ dev_dbg(&desc->intf->dev, "%s: zero length - clearing WDM_READ\n", __func__); clear_bit(WDM_READ, &desc->flags); @@ -844,6 +859,7 @@ static int wdm_post_reset(struct usb_interface *intf) struct wdm_device *desc = usb_get_intfdata(intf); int rv; + clear_bit(WDM_OVERFLOW, &desc->flags); rv = recover_from_urb_loss(desc); mutex_unlock(&desc->plock); return 0;