Re: [PATCH 3/3] usb: usbip: fix OOB read/write in usbip_pad_iso()

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Mar 25, 2026 at 12:26:06PM +0300, Kelvin Mbogo wrote:
> usbip_pad_iso() repositions ISO frame data within the transfer buffer
> via memmove().  Neither the source offset (actualoffset, derived by
> subtracting wire-supplied actual_length values) nor the destination
> offset (iso_frame_desc[i].offset, taken directly from the wire) is
> bounds-checked.
> 
> If a crafted actual_length wraps actualoffset negative through the
> subtraction (see patch 2/3 for the root cause), the memmove source
> points before the allocation - slab OOB read, data returned to
> userspace.
> 
> Independently, iso_frame_desc[i].offset is never validated against
> transfer_buffer_length.  Setting offset past the end of the buffer
> gives a fully controlled OOB write into whatever sits next in the
> slab - confirmed with offset=400 on a 392-byte buffer, 64-byte write.
> 
> Add bounds checks for both the source and destination ranges before
> each memmove call.  Use unsigned comparisons after the sign check on
> actualoffset to avoid signed/unsigned conversion surprises.
> 
> Reported-by: Kelvin Mbogo <addcontent08@gmail.com>
> Signed-off-by: Kelvin Mbogo <addcontent08@gmail.com>
> ---
>  drivers/usb/usbip/usbip_common.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
> index c79a90f..e95e63f 100644
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c
> @@ -769,6 +769,36 @@ void usbip_pad_iso(struct usbip_device *ud, struct urb *urb)
>  	 */
>  	for (i = np-1; i > 0; i--) {
>  		actualoffset -= urb->iso_frame_desc[i].actual_length;
> +
> +		/* source: actualoffset can go negative via crafted
> +		 * actual_length - catch that plus any overshoot */
> +		if (actualoffset < 0 ||
> +		    (unsigned int)actualoffset > (unsigned int)urb->transfer_buffer_length ||
> +		    urb->iso_frame_desc[i].actual_length >
> +				(unsigned int)urb->transfer_buffer_length -
> +				(unsigned int)actualoffset) {


That's soem rough indentation, didn't checkpatch complain about it?

> +			dev_err(&urb->dev->dev,
> +				"pad_iso: bad src off=%d len=%u bufsz=%d\n",
> +				actualoffset,
> +				urb->iso_frame_desc[i].actual_length,
> +				urb->transfer_buffer_length);
> +			return;
> +		}
> +
> +		/* dest: offset comes straight from the wire, never checked */

as you are checking it here, why not say that?

> +		if (urb->iso_frame_desc[i].offset >
> +				(unsigned int)urb->transfer_buffer_length ||
> +		    urb->iso_frame_desc[i].actual_length >
> +				(unsigned int)urb->transfer_buffer_length -
> +				urb->iso_frame_desc[i].offset) {

Again, odd indentation.

thanks,

greg k-h