Re: [PATCH v2] usb: usbtest: reject transfer lengths the allocator cannot serve

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: division by zero in simple_io() when length is 0
- [Medium] usb: usbtest: incomplete limit in iso_alloc_urb allows MAX_PAGE_ORDER bypass
--

commit 805535b4eccf7c27a2100576fca5ef6fb016a6bd
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

usb: usbtest: reject transfer lengths the allocator cannot serve

This commit restricts the length parameter of usbtest ioctls to ensure it
doesn't exceed KMALLOC_MAX_SIZE. It aims to fix warnings from the page
allocator when a large parameter length triggers allocations above
MAX_PAGE_ORDER.

> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index 8759df49be287..6a3394800fd86 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -2192,6 +2192,9 @@ usbtest_do_ioctl(struct usb_interface *intf, struct usbtest_param_32 *param)
>  		return -EINVAL;
>  	if (param->sglen > MAX_SGLEN)
>  		return -EINVAL;
> +	/* Don't allow unreasonably large transfer-buffer allocations */
> +	if (param->length >= KMALLOC_MAX_SIZE)
> +		return -EINVAL;

[Severity: High]
Does this check also need to ensure param->length > 0?

If userspace provides param->length == 0 and param->vary != 0 for test cases
3 or 4, simple_alloc_urb() succeeds and returns an URB with
transfer_buffer_length = 0.

When simple_io() is called:

drivers/usb/misc/usbtest.c:simple_io() {
    ...
	int max = urb->transfer_buffer_length; /* max becomes 0 */
    ...
		if (vary) {
			int len = urb->transfer_buffer_length;

			len += vary;
			len %= max;
    ...
}

Can this division by zero trigger a kernel panic?

[Severity: Medium]
Is this upper bound check sufficient for isochronous transfers in tests 15
and 16?

For these tests, test_queue() calls iso_alloc_urb() which allocates the URB
based on the number of packets:

drivers/usb/misc/usbtest.c:iso_alloc_urb() {
    ...
	maxp = usb_endpoint_maxp(desc);
    ...
	packets = DIV_ROUND_UP(bytes, maxp);
	urb = usb_alloc_urb(packets, GFP_KERNEL);
    ...
}

If a malicious or dummy USB device provides an isochronous endpoint with a
small maxp (e.g., 1), packets will equal param->length. usb_alloc_urb() will
then allocate memory sized for the URB and its packet descriptors:
sizeof(struct urb) + packets * sizeof(struct usb_iso_packet_descriptor).
This is roughly 192 + param->length * 16 bytes.

If param->length is just below KMALLOC_MAX_SIZE, the resulting allocation
size passed to kmalloc() will be roughly 16 times KMALLOC_MAX_SIZE. Does
this exceed MAX_PAGE_ORDER and trigger the exact same warning this patch
attempts to fix?

>  	/*
>  	 * Just a bunch of test cases that every HCD is expected to handle.
>  	 *

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016154707.13098-1-25031212351@stu.xidian.edu.cn?part=1