Re: [PATCH 1/3] usb: usbip: fix integer overflow in usbip_recv_iso()

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Mar 25, 2026 at 12:26:04PM +0300, Kelvin Mbogo wrote:
> usbip_recv_iso() computes the iso descriptor buffer size as:
> 
>     int size = np * sizeof(*iso);
> 
> where np comes straight from the wire (urb->number_of_packets, set by
> usbip_pack_ret_submit() before we get here).  With np = 0x10000001 and
> sizeof(*iso) == 16 the product is 0x100000010 which truncates to 16 on
> a 32-bit int.  kzalloc(16) succeeds but the following receive loop
> writes np * 16 bytes into it - game over.
> 
> USBIP_MAX_ISO_PACKETS (1024) already exists in usbip_common.h for the
> submit path but was never enforced on the receive side.
> 
> Clamp np to [1, USBIP_MAX_ISO_PACKETS] and switch to kmalloc_array()
> so the allocator itself can catch overflows in the future.
> 
> One subtlety: usbip_pack_ret_submit() already copied the bogus np into
> urb->number_of_packets before we run, so just returning -EPROTO isn't
> enough - processcompl() in the HCD will still iterate that many
> iso_frame_desc entries when it completes the failed URB.  Zero out
> urb->number_of_packets before bailing to prevent that secondary crash
> (confirmed on 6.12.0, processcompl+0x63 with CR2 in unmapped slab).
> 
> Reported-by: Kelvin Mbogo <addcontent08@gmail.com>
> Signed-off-by: Kelvin Mbogo <addcontent08@gmail.com>

Nit, no need to have reported-by when you author and sign off on a
patch.

and as this is public, no need to cc: security@k.o anymore either.

> ---
>  drivers/usb/usbip/usbip_common.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
> index a2b2da1..549e34b 100644
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c
> @@ -662,7 +662,6 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
>  	void *buff;
>  	struct usbip_iso_packet_descriptor *iso;
>  	int np = urb->number_of_packets;
> -	int size = np * sizeof(*iso);
>  	int i;
>  	int ret;
>  	int total_length = 0;
> @@ -674,12 +673,22 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
>  	if (np == 0)
>  		return 0;
>  
> -	buff = kzalloc(size, GFP_KERNEL);
> +	if (np < 0 || np > USBIP_MAX_ISO_PACKETS) {
> +		dev_err(&urb->dev->dev,
> +			"recv iso: invalid number_of_packets %d\n", np);
> +		/* usbip_pack_ret_submit() already set urb->number_of_packets
> +		 * from the wire - zero it so processcompl() does not iterate
> +		 * OOB descriptors on the way out. */
> +		urb->number_of_packets = 0;
> +		return -EPROTO;
> +	}

Why not just make the np == 0 do the same here and just silently eat the
message?  Do we have to report an error?

Also, nit, the comment style you used here is for network drivers, not
USB drivers :)


> +
> +	buff = kmalloc_array(np, sizeof(*iso), GFP_KERNEL);

Not kzalloc_objs()?


>  	if (!buff)
>  		return -ENOMEM;
>  
> -	ret = usbip_recv(ud->tcp_socket, buff, size);
> -	if (ret != size) {
> +	ret = usbip_recv(ud->tcp_socket, buff, np * sizeof(*iso));
> +	if (ret != np * (int)sizeof(*iso)) {

is the cast needed?  Or is there a compiler warning without it?

And as you are calculating "np * sizeof(*iso)" multiple times here, why
not just keep the "size" variable at the top here?  Then that would be
one less line changed here.

thanks,

greg k-h