[PATCH] usbip: vudc: snapshot endpoint state under lock

[PATCH] usbip: vudc: snapshot endpoint state under lock

[Sung Byeongchan]

v_recv_cmd_submit() finds an endpoint and records its type while holding
udc->lock, but then drops the lock and rereads both ep->type and ep->desc.
The VUDC timer drops this lock around gadget setup callbacks, and
endpoint disable clears ep->desc under the lock.  A USB/IP host can
therefore race an ISO CMD_SUBMIT with endpoint disable and make
usb_endpoint_maxp() dereference NULL.

Copy the endpoint type and derived isochronous maximum packet size while
the lock still protects the descriptor.  Use the snapshots for
validation, URB allocation, and pipe setup after unlocking.

This was found by source review with AI assistance.  The production VUDC
and UAC2 fixture panicked in two independent boots without a diagnostic
kernel change.  The fixed kernel completed 5,257 race attempts without
an oops or panic and preserved the normal and serialized controls.

Fixes: b78d830f0049 ("usbip: fix vudc_rx: harden CMD_SUBMIT path to handle malicious input")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Sung Byeongchan <tjdqudcks0424@naver.com>
---
 drivers/usb/usbip/vudc_rx.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
index 51bb70837b902..276f645734484 100644
--- a/drivers/usb/usbip/vudc_rx.c
+++ b/drivers/usb/usbip/vudc_rx.c
@@ -91,6 +91,7 @@ static int v_recv_cmd_submit(struct vudc *udc,
 	int ret = 0;
 	struct urbp *urb_p;
 	u8 address;
+	unsigned int maxp = 0;
 	unsigned long flags;
 
 	urb_p = alloc_urbp();
@@ -115,17 +116,19 @@ static int v_recv_cmd_submit(struct vudc *udc,
 		goto free_urbp;
 	}
 	urb_p->type = urb_p->ep->type;
+	if (urb_p->type == USB_ENDPOINT_XFER_ISOC) {
+		maxp = usb_endpoint_maxp(urb_p->ep->desc);
+		maxp *= usb_endpoint_maxp_mult(urb_p->ep->desc);
+	}
 	spin_unlock_irqrestore(&udc->lock, flags);
 
 	urb_p->new = 1;
 	urb_p->seqnum = pdu->base.seqnum;
 
-	if (urb_p->ep->type == USB_ENDPOINT_XFER_ISOC) {
+	if (urb_p->type == USB_ENDPOINT_XFER_ISOC) {
 		/* validate packet size and number of packets */
-		unsigned int maxp, packets, bytes;
+		unsigned int packets, bytes;
 
-		maxp = usb_endpoint_maxp(urb_p->ep->desc);
-		maxp *= usb_endpoint_maxp_mult(urb_p->ep->desc);
 		bytes = pdu->u.cmd_submit.transfer_buffer_length;
 		packets = DIV_ROUND_UP(bytes, maxp);
 
@@ -139,7 +142,7 @@ static int v_recv_cmd_submit(struct vudc *udc,
 		}
 	}
 
-	ret = alloc_urb_from_cmd(&urb_p->urb, pdu, urb_p->ep->type);
+	ret = alloc_urb_from_cmd(&urb_p->urb, pdu, urb_p->type);
 	if (ret) {
 		usbip_event_add(&udc->ud, VUDC_EVENT_ERROR_MALLOC);
 		ret = -ENOMEM;
@@ -152,7 +155,7 @@ static int v_recv_cmd_submit(struct vudc *udc,
 	BUILD_BUG_ON_MSG(PIPE_BULK != 3, "PIPE_* doesn't range from 0 to 3");
 
 	urb_p->urb->pipe &= ~(PIPE_BULK << 30);
-	switch (urb_p->ep->type) {
+	switch (urb_p->type) {
 	case USB_ENDPOINT_XFER_BULK:
 		urb_p->urb->pipe |= (PIPE_BULK << 30);
 		break;
-- 
2.43.0

Re: [PATCH] usbip: vudc: snapshot endpoint state under lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usbip: vudc: memory exhaustion WARN via unvalidated transfer_buffer_length
--

commit 8f3791b0614070178b7b8445680ee73ac6933327
Author: Sung Byeongchan <tjdqudcks0424@naver.com>

usbip: vudc: snapshot endpoint state under lock

v_recv_cmd_submit() finds an endpoint and records its type while holding
udc->lock, but then drops the lock and rereads both ep->type and ep->desc.
The VUDC timer drops this lock around gadget setup callbacks, and
endpoint disable clears ep->desc under the lock.  A USB/IP host can
therefore race an ISO CMD_SUBMIT with endpoint disable and make
usb_endpoint_maxp() dereference NULL.

Copy the endpoint type and derived isochronous maximum packet size while
the lock still protects the descriptor.  Use the snapshots for
validation, URB allocation, and pipe setup after unlocking.

Cc: stable@vger.kernel.org

> diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
> index 51bb70837b902..276f645734484 100644
> --- a/drivers/usb/usbip/vudc_rx.c
> +++ b/drivers/usb/usbip/vudc_rx.c
[ ... ]
> @@ -139,7 +142,7 @@ static int v_recv_cmd_submit(struct vudc *udc,
>  		}
>  	}
>  
> -	ret = alloc_urb_from_cmd(&urb_p->urb, pdu, urb_p->ep->type);
> +	ret = alloc_urb_from_cmd(&urb_p->urb, pdu, urb_p->type);

[Severity: High]
Does this code allow an attacker to trigger a memory exhaustion WARN?

In v_recv_cmd_submit(), there appears to be no bounds check on
transfer_buffer_length for non-ISOC endpoints before it is passed to
alloc_urb_from_cmd().

Inside alloc_urb_from_cmd(), usbip_pack_pdu() copies the signed __s32
transfer_buffer_length field into the unsigned u32
urb->transfer_buffer_length.

If a malicious USB/IP host provides a negative length, such as -1, it would
become a massive u32 value like 0xFFFFFFFF. This bypasses the > 0 check and
is passed directly to kzalloc() with GFP_KERNEL:

alloc_urb_from_cmd() {
    ...
    if (urb->transfer_buffer_length > 0) {
        urb->transfer_buffer = kzalloc(urb->transfer_buffer_length,
            GFP_KERNEL);
        ...
    }
}

Without __GFP_NOWARN, requesting a size larger than KMALLOC_MAX_SIZE causes
the SLUB allocator to trigger a WARN_ON_ONCE_GFP. On systems configured with
panic_on_warn, this could result in a kernel panic and remote
Denial-of-Service.

Can a bounds check be added for non-ISOC endpoints before calling
alloc_urb_from_cmd()?

>  	if (ret) {
>  		usbip_event_add(&udc->ud, VUDC_EVENT_ERROR_MALLOC);
>  		ret = -ENOMEM;

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

Re: [PATCH] usbip: vudc: snapshot endpoint state under lock

[Shuah Khan]

On 10/7/26 00:30, Sung Byeongchan wrote:
> v_recv_cmd_submit() finds an endpoint and records its type while holding
> udc->lock, but then drops the lock and rereads both ep->type and ep->desc.
> The VUDC timer drops this lock around gadget setup callbacks, and
> endpoint disable clears ep->desc under the lock.  A USB/IP host can
> therefore race an ISO CMD_SUBMIT with endpoint disable and make
> usb_endpoint_maxp() dereference NULL.
> 
> Copy the endpoint type and derived isochronous maximum packet size while
> the lock still protects the descriptor.  Use the snapshots for
> validation, URB allocation, and pipe setup after unlocking.
> 
> This was found by source review with AI assistance.  The production VUDC
> and UAC2 fixture panicked in two independent boots without a diagnostic
> kernel change.  The fixed kernel completed 5,257 race attempts without
> an oops or panic and preserved the normal and serialized controls.

This change is incorrect to fix the problem you are describing. Can you
elaborate on how your testing method and environment in more detail?

thanks,
-- Shuah