[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