Re: [PATCH] usbip: avoid dereferencing a missing device in receive errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usbip: integer signedness in usbip_recv_xbuff() allows OOB read
--

commit 02833e7622ef2723c4209138f2286c80d50e18e5
Author: Sung Byeongchan <tjdqudcks0424@naver.com>

usbip: avoid dereferencing a missing device in receive errors

This commit adds NULL checks for urb->dev across several shared receive and
ISO diagnostic paths in USB/IP. This prevents kernel crashes when a missing
device dereference occurs, such as when processing synthetic URBs built by
VUDC.

> diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
> index 100f8920624bb..8d379a9437efa 100644
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c

[ ... ]

> @@ -891,7 +897,9 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
>  	return ret;
>  
>  error:
> -	dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> +	/* VUDC creates synthetic URBs without a struct usb_device. */
> +	if (urb->dev)
> +		dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);

[Severity: High]
Does the signedness of the size variable earlier in usbip_recv_xbuff() allow
an out-of-bounds read?

When handling the actual length, urb->actual_length (which is a u32) is
implicitly converted to a signed int:

	} else {
		/* the direction of urb must be IN. */
		if (usb_pipeout(urb->pipe))
			return 0;

		size = urb->actual_length;
	}

	/* no need to recv xbuff */
	if (!(size > 0))
		return 0;

	if (size > urb->transfer_buffer_length)
		/* should not happen, probably malicious packet */
		goto error;

If a malicious USB/IP server sends a USBIP_RET_SUBMIT packet with an
actual_length greater than 0x7FFFFFFF, size will become negative.

The check !(size > 0) will then evaluate to true, causing the function to
immediately return 0 and bypass the bounds check against
urb->transfer_buffer_length that normally jumps to this error path.

Could this allow the client driver to process the buffer using the forged
actual_length, resulting in an out-of-bounds read beyond its allocated
transfer_buffer?

>  	if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC)
>  		usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
>  	else

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008134107.265827-1-tjdqudcks0424@naver.com?part=1