[PATCH] usbip: vudc: reject submits after endpoint state changes

[PATCH] usbip: vudc: reject submits after endpoint state changes

[tjdqudcks0424]

From: Sung Byeongchan <tjdqudcks0424@naver.com>

v_recv_cmd_submit() looks up an endpoint under udc->lock, but drops the
lock while it allocates the URB and receives the payload.  A gadget
configuration change can disable or disable and re-enable that endpoint
before the URB is queued.  The submit then carries stale endpoint state
into asynchronous VUDC processing.  The confirmed failure dereferences a
NULL urb->dev while logging a completion error and panics the kernel.

Give each VUDC endpoint a generation that changes on enable and disable.
Snapshot the generation and immutable parsing fields during lookup, then
recheck descriptor presence and generation under udc->lock immediately
before adding the URB to urb_queue.  Reject a stale request with
-ESHUTDOWN.

On a KASAN build at ff47652a4b66c067c765a7ad464d930b5a9367cc,
normal enabled-endpoint, serialized-disable, and disable/re-enable
stale-submit cases each passed three independent fresh-boot runs after the
fix.  The stale case returned -ESHUTDOWN without a sanitizer report, and a
following EP0 request succeeded after bounded reconnection.  The touched
source is unchanged at 0c2669a9f4a1d607e7591ae50ccf3c432a0aff08, where
the patch also applies and builds cleanly.

The demonstrated impact is kernel/service denial of service.  No controlled
write, information disclosure, RCE, or LPE was demonstrated.  The source
reproducer and complete logs are available on request and are not included
in this public report.

OpenAI Codex assisted source inspection, test design, patch drafting, and
evidence organization.

Fixes: 79c02cb1fd5c ("usbip: vudc: Add vudc_rx")
Assisted-by: LLM
Signed-off-by: Sung Byeongchan <tjdqudcks0424@naver.com>
---
 drivers/usb/usbip/vudc.h     |  3 +++
 drivers/usb/usbip/vudc_dev.c |  2 ++
 drivers/usb/usbip/vudc_rx.c  | 21 ++++++++++++++++-----
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/usbip/vudc.h b/drivers/usb/usbip/vudc.h
index 5ef0e7d9b23ad..b2adc61f181cb 100644
--- a/drivers/usb/usbip/vudc.h
+++ b/drivers/usb/usbip/vudc.h
@@ -28,6 +28,7 @@ struct vep {
 	char name[8];	/* space for ep name */
 
 	const struct usb_endpoint_descriptor *desc;
+	u64 generation;
 	struct usb_gadget *gadget;
 	struct list_head req_queue; /* Request queue */
 	unsigned halted:1;
@@ -44,6 +45,8 @@ struct vrequest {
 struct urbp {
 	struct urb *urb;
 	struct vep *ep;
+	u64 ep_generation;
+	unsigned int ep_maxp;
 	struct list_head urb_entry; /* urb queue */
 	unsigned long seqnum;
 	unsigned type:2; /* for tx, since ep type can change after */
diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 5ef88117965d1..e3800d5e83a30 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -250,6 +250,7 @@ static int vep_enable(struct usb_ep *_ep,
 	_ep->maxpacket = maxp;
 	ep->desc = desc;
 	ep->type = usb_endpoint_type(desc);
+	ep->generation++;
 	ep->halted = ep->wedged = 0;
 
 	spin_unlock_irqrestore(&udc->lock, flags);
@@ -270,6 +271,7 @@ static int vep_disable(struct usb_ep *_ep)
 
 	spin_lock_irqsave(&udc->lock, flags);
 	ep->desc = NULL;
+	ep->generation++;
 	nuke(udc, ep);
 	spin_unlock_irqrestore(&udc->lock, flags);
 
diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
index 51bb70837b902..3c459c7b57b5f 100644
--- a/drivers/usb/usbip/vudc_rx.c
+++ b/drivers/usb/usbip/vudc_rx.c
@@ -115,17 +115,21 @@ static int v_recv_cmd_submit(struct vudc *udc,
 		goto free_urbp;
 	}
 	urb_p->type = urb_p->ep->type;
+	urb_p->ep_generation = urb_p->ep->generation;
+	if (urb_p->type == USB_ENDPOINT_XFER_ISOC) {
+		urb_p->ep_maxp = usb_endpoint_maxp(urb_p->ep->desc);
+		urb_p->ep_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;
 
-		maxp = usb_endpoint_maxp(urb_p->ep->desc);
-		maxp *= usb_endpoint_maxp_mult(urb_p->ep->desc);
+		maxp = urb_p->ep_maxp;
 		bytes = pdu->u.cmd_submit.transfer_buffer_length;
 		packets = DIV_ROUND_UP(bytes, maxp);
 
@@ -139,7 +143,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 +156,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;
@@ -175,6 +179,13 @@ static int v_recv_cmd_submit(struct vudc *udc,
 		goto free_urbp;
 
 	spin_lock_irqsave(&udc->lock, flags);
+	if (urb_p->ep != &udc->ep[0] &&
+	    (!urb_p->ep->desc ||
+	     urb_p->ep->generation != urb_p->ep_generation)) {
+		spin_unlock_irqrestore(&udc->lock, flags);
+		ret = -ESHUTDOWN;
+		goto free_urbp;
+	}
 	v_kick_timer(udc, jiffies);
 	list_add_tail(&urb_p->urb_entry, &udc->urb_queue);
 	spin_unlock_irqrestore(&udc->lock, flags);
-- 
2.43.0

Re: [PATCH] usbip: vudc: reject submits after endpoint state changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usbip: vudc: connection state leak on stale URB submit
--

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

usbip: vudc: reject submits after endpoint state changes

v_recv_cmd_submit() looks up an endpoint under udc->lock, but drops the
lock while it allocates the URB and receives the payload. A gadget
configuration change can disable or disable and re-enable that endpoint
before the URB is queued. This commit adds a generation counter to reject
stale requests with -ESHUTDOWN.

> diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
> index 51bb70837b902..3c459c7b57b5f 100644
> --- a/drivers/usb/usbip/vudc_rx.c
> +++ b/drivers/usb/usbip/vudc_rx.c
[ ... ]
> @@ -175,6 +179,13 @@ static int v_recv_cmd_submit(struct vudc *udc,
>  		goto free_urbp;
>  
>  	spin_lock_irqsave(&udc->lock, flags);
> +	if (urb_p->ep != &udc->ep[0] &&
> +	    (!urb_p->ep->desc ||
> +	     urb_p->ep->generation != urb_p->ep_generation)) {
> +		spin_unlock_irqrestore(&udc->lock, flags);
> +		ret = -ESHUTDOWN;
> +		goto free_urbp;
> +	}

[Severity: Medium]
Does this new error path omit a required usbip_event_add() call?

When this error path in v_recv_cmd_submit() returns -ESHUTDOWN, the negative
return value propagates to v_rx_pdu() and then to v_rx_loop(), causing the RX
thread loop to break and the thread to exit.

Because the usbip_event_add(&udc->ud, VUDC_EVENT_ERROR_TCP) event notification
is missing here, the usbip_work teardown routine is never scheduled. As a
result, the TX thread remains indefinitely blocked on tx_waitq, the socket
reference is not released, and the device state remains stuck in SDEV_ST_USED.

Could this be triggered when a host submits an URB to an endpoint whose
generation has changed, leaking the connection state on the server until manual
administrative intervention?

>  	v_kick_timer(udc, jiffies);
>  	list_add_tail(&urb_p->urb_entry, &udc->urb_queue);
>  	spin_unlock_irqrestore(&udc->lock, flags);

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

Re: [PATCH] usbip: vudc: reject submits after endpoint state changes

[Shuah Khan]

On 10/8/26 12:37, tjdqudcks0424@naver.com wrote:
> From: Sung Byeongchan <tjdqudcks0424@naver.com>
> 
> v_recv_cmd_submit() looks up an endpoint under udc->lock, but drops the
> lock while it allocates the URB and receives the payload.  A gadget
> configuration change can disable or disable and re-enable that endpoint
> before the URB is queued.  The submit then carries stale endpoint state
> into asynchronous VUDC processing.  The confirmed failure dereferences a
> NULL urb->dev while logging a completion error and panics the kernel.
> 
> Give each VUDC endpoint a generation that changes on enable and disable.
> Snapshot the generation and immutable parsing fields during lookup, then
> recheck descriptor presence and generation under udc->lock immediately
> before adding the URB to urb_queue.  Reject a stale request with
> -ESHUTDOWN.
> 
> On a KASAN build at ff47652a4b66c067c765a7ad464d930b5a9367cc,
> normal enabled-endpoint, serialized-disable, and disable/re-enable
> stale-submit cases each passed three independent fresh-boot runs after the
> fix.  The stale case returned -ESHUTDOWN without a sanitizer report, and a
> following EP0 request succeeded after bounded reconnection.  The touched
> source is unchanged at 0c2669a9f4a1d607e7591ae50ccf3c432a0aff08, where
> the patch also applies and builds cleanly.
> 
> The demonstrated impact is kernel/service denial of service.  No controlled
> write, information disclosure, RCE, or LPE was demonstrated.  The source
> reproducer and complete logs are available on request and are not included
> in this public report.
> 
> OpenAI Codex assisted source inspection, test design, patch drafting, and
> evidence organization.

Please explain how this problem can be reproduced and why this fix
masese sense?

thanks,
-- Shuah

Re: [PATCH] usbip: vudc: reject submits after endpoint state changes

[성병찬]

Hi Shuah,

Thank you for asking. I need to correct one part of my original commit
message before explaining the reproduction.

The sentence attributing the race-specific failure to the NULL urb->dev
diagnostic was inaccurate. That diagnostic is the separately confirmed parent
VUDC receive-error issue. In the endpoint-state race described by this patch,
the observed crash occurred in the v_recv_cmd_submit() path after the endpoint
descriptor had been cleared.

I reproduced the race in a network-isolated x86_64 QEMU guest at
ff47652a4b66c067c765a7ad464d930b5a9367cc with KASAN enabled. The PID-1
fixture creates a real configfs UAC2 gadget and attaches a local socketpair to
the production VUDC usbip_sockfd interface. It does not call an internal VUDC
function directly.

For the stale-submit case, the fixture performs the following sequence:

  1. Enable and enumerate an isochronous OUT endpoint.
  2. Send a SET_INTERFACE request selecting altsetting 0, immediately followed
     by the header of an ISO OUT CMD_SUBMIT, in one socket write.
  3. Let the receive thread consume the ISO submit header and begin waiting for
     its body.
  4. While it is waiting, let the VUDC transfer timer complete SET_INTERFACE
     and disable the endpoint.
  5. Send the delayed ISO body after the endpoint has been disabled.
  6. Re-enable the same logical endpoint and check the stale completion and a
     following EP0 operation.

There is no kernel-side delay or test hook. The receive thread is held in the
normal payload receive operation while the normal VUDC timer processes the
endpoint transition.

The baseline race is timing-sensitive. Five of six baseline stale runs
recovered without a sanitizer report, but one run produced a KASAN NULL pointer
dereference in the inlined v_recv_cmd_submit() path. This is consistent with
the endpoint descriptor becoming NULL while the unlocked code calculates the
endpoint maximum packet size. The other runs still demonstrated that the
submit could cross the endpoint state transition.

The generation check is intended to close both parts of that window:

  - endpoint type and maximum packet size are snapshotted while udc->lock is
    held, so parsing does not dereference a mutable descriptor after dropping
    the lock;
  - enable and disable each increment the endpoint generation while holding
    the same lock;
  - immediately before urb_queue insertion, the receiver reacquires
    udc->lock and requires both a present descriptor and the same generation.

Checking only the endpoint pointer would not be sufficient because disable and
re-enable reuse the same struct vep. The generation check distinguishes the
new endpoint state from the state used when the submit was parsed. -ESHUTDOWN
is returned because the endpoint instance for which the submit was received no
longer exists.

Using the exact code diff that I posted, I ran three fresh boots each for
normal operation, serialized disable, and the stale-generation sequence. The
three stale runs sent the body after disable, were rejected with -ESHUTDOWN,
remained free of KASAN/oops/panic reports, and completed a following EP0
request after bounded transport recovery.

I will correct the failure description in a v2 commit message. The code diff
will remain unchanged.

Thanks,
Sung byeongchan