On Fri, 9 Dec 2016, Mateusz Berezecki wrote:
> On 9 Dec 2016, at 13:50, Alan Stern wrote:
>
> > On Fri, 9 Dec 2016, Mateusz Berezecki wrote:
> >
> >> /* Limit on the total amount of memory we can allocate for transfers
> >> */
> >> -static unsigned usbfs_memory_mb = 16;
> >> +static u32 usbfs_memory_mb = 16;
> >> 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)
> >> +/* Hard limit */
> >> +#define USBFS_XFER_MAX (1ull << 52)
> >
> > I should have realized this before -- my fault. Since usbfs_memory_mb
> > is a u32 value, it can never be >= 2^32 MB. Consequently, the total
> > memory limit can never be >= 2^52 bytes, so there's no need for
> > USBFS_XFER_MAX at all!
> >
> > The only reason it's there now is, like the comment says, with 32-bit
> > values the user could cause an arithmetic overflow. With 64-bit
> > values
> > that isn't possible.
> >
> > So you might as well just get rid of it entirely.
>
> That’s what I thought initially as well, but in the proc_bulk()
> function, there is this piece :
>
> static int proc_bulk(struct usb_dev_state *ps, void __user *arg)
> {
> [...]
> struct usbdevfs_bulktransfer bulk;
> unsigned int tmo, len1, pipe;
>
> [...]
>
> if (copy_from_user(&bulk, arg, sizeof(bulk)))
> return -EFAULT;
>
> [...]
>
> if (!usb_maxpacket(dev, pipe, !(bulk.ep & USB_DIR_IN)))
> return -EINVAL;
> len1 = bulk.len;
> if (len1 >= USBFS_XFER_MAX)
> return -EINVAL;
> ret = usbfs_increase_memory_usage(len1 + sizeof(struct urb));
> if (ret)
> return ret;
> tbuf = kmalloc(len1, GFP_KERNEL);
>
> Does this potentially open up some sort of resource-exhaustion
> vulnerability?
No. Although we should be careful, because len1 + sizeof(struct urb)
could overflow an unsigned int.
> Specifically, the part where bulk is copied in from userspace,
> and the len1 = bulk.len . Next without checking for some bounds,
> we’d just add up that value to the memory counter.
If the memory counter is a 32-bit value then you're right; the addition
could overflow the counter. That's why the check is there now.
But if you change the counter to a 64-bit value then there's no
possibility of overflow, because len1 and bulk.len are both 32-bit
quantities. The sum of two values, where one is limited to 2^52 and
the other is limited to 2^32, cannot overflow a 64-bit word.
> Should the limit stay, perhaps?
Not in the form it is now. Really, the only need is to make sure that
len1 + sizeof(struct urb) won't overflow a 32-bit word.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
