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?
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.
Should the limit stay, perhaps?
Mateusz
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
