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

[PATCH v2] 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.

In a timing-controlled KASAN test, one of six unmodified baseline runs
faulted in the inlined receive path while reading endpoint descriptor
state after the descriptor had been cleared.  The other baseline runs
also showed that a submit could cross the endpoint state transition.

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.

The parsing snapshots avoid unlocked reads of mutable descriptor state.
The generation check also distinguishes a newly enabled endpoint from the
old endpoint state even though both use the same struct vep.

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.

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>
---
Changes in v2:
- Correct the race-specific NULL dereference description.
- Document the production-path reproduction and timing-sensitive baseline result.
- Explain why generation revalidation closes disable/re-enable reuse.
- No code changes.

 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 v2] usbip: vudc: reject submits after endpoint state changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usbip: vudc: v_recv_cmd_submit() -ESHUTDOWN return wedges connection
- [Medium] usbip: vudc: v_timer() and vep_disable() ignore queued URB generation
--

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

usbip: vudc: reject submits after endpoint state changes

This commit adds a per-endpoint generation counter to vudc endpoints and
snapshots endpoint state under udc->lock in v_recv_cmd_submit(). Before
queuing an incoming urb_p onto udc->urb_queue, it revalidates that the
endpoint descriptor is still present and the generation has not changed.

> 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

[ ... ]

> @@ -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);

[Severity: Medium]
What happens to urbs that were already validated and added to
udc->urb_queue before vep_disable() runs?

In drivers/usb/usbip/vudc_dev.c, vep_disable() clears ep->desc, increments
ep->generation, and calls nuke(udc, ep), which only drains gadget requests
from ep->req_queue while leaving udc->urb_queue untouched.

When v_timer() in drivers/usb/usbip/vudc_transfer.c iterates
udc->urb_queue, it neither checks ep->desc nor compares ep->generation
against urb_p->ep_generation:

drivers/usb/usbip/vudc_transfer.c:v_timer() {
	...
	list_for_each_entry_safe(urb_p, tmp, &udc->urb_queue, urb_entry) {
		struct urb *urb = urb_p->urb;

		ep = urb_p->ep;
		if (urb->unlinked)
			goto return_urb;
		if (timer->state != VUDC_TR_RUNNING)
			continue;

		if (!ep) {
			urb->status = -EPROTO;
			goto return_urb;
		}
		...
		switch (ep->type) {
		...
		default:
treat_control_like_bulk:
			total -= transfer(udc, urb, ep, limit);
		}
		if (urb->status == -EINPROGRESS)
			continue;
}

If the endpoint remains disabled, transfer() sees an empty ep->req_queue
and leaves urb->status unchanged, keeping urb_p queued on udc->urb_queue
and re-arming tr_timer every millisecond. If the endpoint is later
re-enabled via vep_enable(), v_timer() processes the stale urb_p from the
old generation against the new endpoint state and dispatches on the new
ep->type while v_send_ret_submit() uses the snapshotted urb_p->type.

Should vep_disable() or v_timer() also fail queued entries on
udc->urb_queue whose endpoint descriptor was cleared or whose
ep->generation no longer matches urb_p->ep_generation?

> 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;
> +	}
>  	v_kick_timer(udc, jiffies);
>  	list_add_tail(&urb_p->urb_entry, &udc->urb_queue);
>  	spin_unlock_irqrestore(&udc->lock, flags);

[Severity: High]
Can returning this error from v_recv_cmd_submit() wedge the usbip
connection and vudc device state?

By the time v_recv_cmd_submit() reaches this check in
drivers/usb/usbip/vudc_rx.c, usbip_recv_xbuff() and usbip_recv_iso() have
already consumed the full submit payload from ud->tcp_socket. Jumping to
free_urbp drops urb_p without queuing a completion via
v_enqueue_ret_submit() and returns the negative error through v_rx_pdu() to
v_rx_loop():

drivers/usb/usbip/vudc_rx.c:v_rx_pdu() {
	...
	case USBIP_CMD_SUBMIT:
		ret = v_recv_cmd_submit(udc, &pdu);
		break;
	...
	return ret;
}

drivers/usb/usbip/vudc_rx.c:v_rx_loop() {
	...
	while (!kthread_should_stop()) {
		if (usbip_event_happened(ud))
			break;
		ret = v_rx_pdu(ud);
		if (ret < 0) {
			pr_warn("v_rx exit with error %d", ret);
			break;
		}
	}
	return ret;
}

Unlike other fatal error paths in v_recv_cmd_submit() and v_rx_pdu(), this
path does not call usbip_event_add(), so vudc_shutdown() and
vudc_device_reset() in drivers/usb/usbip/vudc_dev.c are never scheduled:

drivers/usb/usbip/vudc_dev.c:vudc_shutdown() {
	...
	if (ud->tcp_socket)
		kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);

	if (ud->tcp_rx) {
		kthread_stop_put(ud->tcp_rx);
		ud->tcp_rx = NULL;
	}
	if (ud->tcp_tx) {
		kthread_stop_put(ud->tcp_tx);
		ud->tcp_tx = NULL;
	}
	...
}

When ud->tcp_rx exits on its own without an event queued, ud->tcp_tx
remains asleep on udc->tx_waitq, ud->tcp_socket stays open with no reader,
the client hangs waiting for a submit reply, and udc->connected and
ud->status remain stuck in the active state.

Since the stream framing on ud->tcp_socket is still intact after reading
the payload, should v_recv_cmd_submit() complete the rejected urb_p back to
the client via v_enqueue_ret_submit() and return 0, or trigger connection
teardown via usbip_event_add()?

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

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

[Shuah Khan]

On 10/20/26 12:18, 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.
> 
> In a timing-controlled KASAN test, one of six unmodified baseline runs
> faulted in the inlined receive path while reading endpoint descriptor
> state after the descriptor had been cleared.  The other baseline runs
> also showed that a submit could cross the endpoint state transition.
> 
> 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.
> 
> The parsing snapshots avoid unlocked reads of mutable descriptor state.
> The generation check also distinguishes a newly enabled endpoint from the
> old endpoint state even though both use the same struct vep.
> 
> 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.

How did you reproduce it? If you didn't how are you sure this is the
right fix?

thanks,
-- Shuah