* [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb()
@ 2017-09-21 7:16 Dan Carpenter
2017-09-21 14:17 ` Alan Stern
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-09-21 7:16 UTC (permalink / raw)
To: kernel-janitors
There used to be an integer overflow check in proc_do_submiturb() but
it accidentally got removed. We need to put it back. The
uurb->buffer_length variable is a signed integer and it's controlled by
the user. It can lead to an integer overflow when we do:
num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
Fixes: 1129d270cbfb ("USB: Increase usbfs transfer limit")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 318bb3b96687..f3c9cff2101c 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -140,6 +140,9 @@ module_param(usbfs_memory_mb, uint, 0644);
MODULE_PARM_DESC(usbfs_memory_mb,
"maximum MB allowed for usbfs buffers (0 = no limit)");
+/* Hard limit, necessary to avoid arithmetic overflow */
+#define USBFS_XFER_MAX (UINT_MAX / 2 - 1000000)
+
static atomic64_t usbfs_memory_usage; /* Total memory currently allocated */
/* Check whether it's okay to allocate more memory for a transfer */
@@ -1460,6 +1463,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
USBDEVFS_URB_ZERO_PACKET |
USBDEVFS_URB_NO_INTERRUPT))
return -EINVAL;
+ if (uurb->buffer_length >= USBFS_XFER_MAX)
+ return -EINVAL;
if (uurb->buffer_length > 0 && !uurb->buffer)
return -EINVAL;
if (!(uurb->type = USBDEVFS_URB_TYPE_CONTROL &&
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb()
2017-09-21 7:16 [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb() Dan Carpenter
@ 2017-09-21 14:17 ` Alan Stern
2017-09-21 14:27 ` Dan Carpenter
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2017-09-21 14:17 UTC (permalink / raw)
To: kernel-janitors
On Thu, 21 Sep 2017, Dan Carpenter wrote:
> There used to be an integer overflow check in proc_do_submiturb() but
> it accidentally got removed. We need to put it back. The
The removal was deliberate, not accidental. :-)
> uurb->buffer_length variable is a signed integer and it's controlled by
> the user. It can lead to an integer overflow when we do:
>
> num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
Sorry, I don't understand. How can division by 16384 lead to an
integer overflow?
Now, I agree that uurb->buffer_length can cause problems. We don't
check for meaningless negative values on all the execution paths (the
field should have been unsigned from the beginning). And in the
USBDEVFS_URB_TYPE_ISO case, we overwrite uurb->buffer_length without
checking that the new value is <= the old value, which could lead to a
userspace buffer overflow.
Alan Stern
> Fixes: 1129d270cbfb ("USB: Increase usbfs transfer limit")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 318bb3b96687..f3c9cff2101c 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -140,6 +140,9 @@ module_param(usbfs_memory_mb, uint, 0644);
> MODULE_PARM_DESC(usbfs_memory_mb,
> "maximum MB allowed for usbfs buffers (0 = no limit)");
>
> +/* Hard limit, necessary to avoid arithmetic overflow */
> +#define USBFS_XFER_MAX (UINT_MAX / 2 - 1000000)
> +
> static atomic64_t usbfs_memory_usage; /* Total memory currently allocated */
>
> /* Check whether it's okay to allocate more memory for a transfer */
> @@ -1460,6 +1463,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
> USBDEVFS_URB_ZERO_PACKET |
> USBDEVFS_URB_NO_INTERRUPT))
> return -EINVAL;
> + if (uurb->buffer_length >= USBFS_XFER_MAX)
> + return -EINVAL;
> if (uurb->buffer_length > 0 && !uurb->buffer)
> return -EINVAL;
> if (!(uurb->type = USBDEVFS_URB_TYPE_CONTROL &&
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb()
2017-09-21 7:16 [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb() Dan Carpenter
2017-09-21 14:17 ` Alan Stern
@ 2017-09-21 14:27 ` Dan Carpenter
2017-09-21 14:40 ` Alan Stern
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-09-21 14:27 UTC (permalink / raw)
To: kernel-janitors
On Thu, Sep 21, 2017 at 10:17:03AM -0400, Alan Stern wrote:
> On Thu, 21 Sep 2017, Dan Carpenter wrote:
>
> > There used to be an integer overflow check in proc_do_submiturb() but
> > it accidentally got removed. We need to put it back. The
>
> The removal was deliberate, not accidental. :-)
>
> > uurb->buffer_length variable is a signed integer and it's controlled by
> > the user. It can lead to an integer overflow when we do:
> >
> > num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
>
> Sorry, I don't understand. How can division by 16384 lead to an
> integer overflow?
It looks like this when you unwrap the macro:
num_sgs = (uurb->buffer_length + USB_SG_SIZE - 1) / USB_SG_SIZE;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This first addition can overflow.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb()
2017-09-21 7:16 [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb() Dan Carpenter
2017-09-21 14:17 ` Alan Stern
2017-09-21 14:27 ` Dan Carpenter
@ 2017-09-21 14:40 ` Alan Stern
2017-09-21 15:05 ` Dan Carpenter
2017-09-21 15:33 ` Alan Stern
4 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2017-09-21 14:40 UTC (permalink / raw)
To: kernel-janitors
On Thu, 21 Sep 2017, Dan Carpenter wrote:
> On Thu, Sep 21, 2017 at 10:17:03AM -0400, Alan Stern wrote:
> > On Thu, 21 Sep 2017, Dan Carpenter wrote:
> >
> > > There used to be an integer overflow check in proc_do_submiturb() but
> > > it accidentally got removed. We need to put it back. The
> >
> > The removal was deliberate, not accidental. :-)
> >
> > > uurb->buffer_length variable is a signed integer and it's controlled by
> > > the user. It can lead to an integer overflow when we do:
> > >
> > > num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
> >
> > Sorry, I don't understand. How can division by 16384 lead to an
> > integer overflow?
>
> It looks like this when you unwrap the macro:
>
> num_sgs = (uurb->buffer_length + USB_SG_SIZE - 1) / USB_SG_SIZE;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This first addition can overflow.
Ah, very good point. Okay:
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Would you like to submit a second patch addressing the other problems I
outlined?
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb()
2017-09-21 7:16 [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb() Dan Carpenter
` (2 preceding siblings ...)
2017-09-21 14:40 ` Alan Stern
@ 2017-09-21 15:05 ` Dan Carpenter
2017-09-21 15:33 ` Alan Stern
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-09-21 15:05 UTC (permalink / raw)
To: kernel-janitors
On Thu, Sep 21, 2017 at 10:17:03AM -0400, Alan Stern wrote:
> On Thu, 21 Sep 2017, Dan Carpenter wrote:
>
> > There used to be an integer overflow check in proc_do_submiturb() but
> > it accidentally got removed. We need to put it back. The
>
> The removal was deliberate, not accidental. :-)
>
> > uurb->buffer_length variable is a signed integer and it's controlled by
> > the user. It can lead to an integer overflow when we do:
> >
> > num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
>
> Sorry, I don't understand. How can division by 16384 lead to an
> integer overflow?
>
> Now, I agree that uurb->buffer_length can cause problems. We don't
> check for meaningless negative values on all the execution paths (the
> field should have been unsigned from the beginning).
Checking against USBFS_XFER_MAX prevents negative values but I'll add
another check for readability.
> And in the
> USBDEVFS_URB_TYPE_ISO case, we overwrite uurb->buffer_length without
> checking that the new value is <= the old value, which could lead to a
> userspace buffer overflow.
Hm... Yeah. I'm not sure what's the right thing, should I print a
warning if we truncate the output or would users figure it out on their
own?
if (totlen <= uurb->buffer_length)
uurb->buffer_length = totlen;
else
WARN_ONCE(1, "uurb->buffer_length is too short %d vs %d",
totlen, uurb->buffer_length);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb()
2017-09-21 7:16 [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb() Dan Carpenter
` (3 preceding siblings ...)
2017-09-21 15:05 ` Dan Carpenter
@ 2017-09-21 15:33 ` Alan Stern
4 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2017-09-21 15:33 UTC (permalink / raw)
To: kernel-janitors
On Thu, 21 Sep 2017, Dan Carpenter wrote:
> On Thu, Sep 21, 2017 at 10:17:03AM -0400, Alan Stern wrote:
> > On Thu, 21 Sep 2017, Dan Carpenter wrote:
> >
> > > There used to be an integer overflow check in proc_do_submiturb() but
> > > it accidentally got removed. We need to put it back. The
> >
> > The removal was deliberate, not accidental. :-)
> >
> > > uurb->buffer_length variable is a signed integer and it's controlled by
> > > the user. It can lead to an integer overflow when we do:
> > >
> > > num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
> >
> > Sorry, I don't understand. How can division by 16384 lead to an
> > integer overflow?
> >
> > Now, I agree that uurb->buffer_length can cause problems. We don't
> > check for meaningless negative values on all the execution paths (the
> > field should have been unsigned from the beginning).
>
> Checking against USBFS_XFER_MAX prevents negative values but I'll add
> another check for readability.
The fact that USBFS_XFER_MAX generates an unsigned comparison is
awfully subtle. However, if you explicitly cast uurb->buffer_length to
unsigned int in the test, then it should be clear that the test also
catches negative values. Maybe adding a comment would be good, but I
think with the cast, no additional check is needed.
> > And in the
> > USBDEVFS_URB_TYPE_ISO case, we overwrite uurb->buffer_length without
> > checking that the new value is <= the old value, which could lead to a
> > userspace buffer overflow.
>
> Hm... Yeah. I'm not sure what's the right thing, should I print a
> warning if we truncate the output or would users figure it out on their
> own?
> if (totlen <= uurb->buffer_length)
> uurb->buffer_length = totlen;
> else
> WARN_ONCE(1, "uurb->buffer_length is too short %d vs %d",
> totlen, uurb->buffer_length);
I like the WARN_ONCE.
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-21 15:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-21 7:16 [PATCH] USB: devio: Prevent integer overflow in proc_do_submiturb() Dan Carpenter
2017-09-21 14:17 ` Alan Stern
2017-09-21 14:27 ` Dan Carpenter
2017-09-21 14:40 ` Alan Stern
2017-09-21 15:05 ` Dan Carpenter
2017-09-21 15:33 ` Alan Stern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.