[PATCH v2] usb: gadget: f_tcm: reject a short write data-out transfer

[PATCH v2] usb: gadget: f_tcm: reject a short write data-out transfer

[Yehyeong Lee]

usbg_data_write_cmpl() commits the WRITE buffer to the backend as soon as
the OUT request completes successfully, looking only at req->status and
never at req->actual.  A host that declares an 8192-byte WRITE but ends the
data-out after 4096 bytes still reaches target_execute_cmd() with the full
length.

The buffer is not zeroed (transport_generic_new_cmd() leaves zero_flag
clear for a data CDB), so the bytes that never arrived keep the previous
page contents, which the backend writes to the device for the host to read
back. The target core refuses a WRITE whose declared length is wrong, but
it does not see a data phase that simply falls short.

Fail the command when the data-out is short.  The OUT data phase is over at
this point, so clear cmd->data_len too: otherwise the BOT status phase
treats it as outstanding WRITE data and queues another OUT request to drain
a host that has already stopped, wedging the bulk pipe.  Rate-limit the
failure message, which a host can now trigger at will.

There is no oops; poisoning the buffer and ending an 8192-byte write at
4096 bytes leaves 4096 stale bytes committed before the change, and after
it the command is aborted with nothing committed.

Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
v2: on a short data-out also clear cmd->data_len so the BOT status phase
    does not queue an OUT drain for a host that already stopped (would
    wedge the bulk pipe, reported by Sashiko); rate-limit the failure log.

 drivers/usb/gadget/function/f_tcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 9e6d4f39900a2..cbe64f934724e 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1090,8 +1090,14 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 		return;
 	}
 
-	if (req->status) {
-		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
+	/* a short data-out left the buffer unwritten; don't commit stale data */
+	if (req->status || req->actual != req->length) {
+		struct usb_gadget *gadget = fuas_to_gadget(cmd->fu);
+
+		/* the OUT data phase is over; nothing left for the status phase to drain */
+		cmd->data_len = 0;
+		dev_err_ratelimited(&gadget->dev, "%s() state %d transfer failed\n",
+				    __func__, cmd->state);
 		goto cleanup;
 	}
 
-- 
2.43.0

Re: [PATCH v2] usb: gadget: f_tcm: reject a short write data-out transfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: gadget: f_tcm: data_len not cleared on successful OUT transfer wedges pipe on backend failure
--

commit 63110d12dd83cfabcbf4574d13f034b5398b8e04
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

usb: gadget: f_tcm: reject a short write data-out transfer

This patch fails the command when a short write data-out occurs, clearing
cmd->data_len to prevent the BOT status phase from treating it as outstanding
WRITE data and queuing another OUT request.

> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 9e6d4f39900a2..cbe64f934724e 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1090,8 +1090,14 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
>  		return;
>  	}
>  
> -	if (req->status) {
> -		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
> +	/* a short data-out left the buffer unwritten; don't commit stale data */
> +	if (req->status || req->actual != req->length) {
> +		struct usb_gadget *gadget = fuas_to_gadget(cmd->fu);
> +
> +		/* the OUT data phase is over; nothing left for the status phase to drain */
> +		cmd->data_len = 0;
> +		dev_err_ratelimited(&gadget->dev, "%s() state %d transfer failed\n",
> +				    __func__, cmd->state);
>  		goto cleanup;
>  	}

[Severity: High]
Does cmd->data_len also need to be cleared for successful full OUT transfers?

If a full OUT transfer succeeds in usbg_data_write_cmpl() (req->status == 0
and req->actual == req->length), the request is submitted to the target core
backend. If the backend fails during execution (e.g., media error, ENOSPC) and
returns a check condition, usbg_send_status_response() routes to
bot_send_status() and subsequently to bot_send_bad_status().

Since cmd->data_len was not cleared, bot_send_bad_status() will attempt to
queue a drain OUT request:

drivers/usb/gadget/function/f_tcm.c:bot_send_bad_status() {
	if (cmd->data_len) {
		...
		if (cmd->data_len > fu->ep_in->maxpacket) {
			...
		} else {
			req->length = cmd->data_len;
			cmd->data_len = 0;
		}
		...
		usb_ep_queue(ep, req, GFP_KERNEL);
	} else {
		bot_enqueue_sense_code(fu, cmd);
	}
}

However, the host has already finished sending all data during the data phase
and is waiting for the CSW on the IN endpoint. Queuing an OUT request here
will hang indefinitely, wedging the bulk pipe and causing a DoS for the
endpoint.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013005854.2470384-1-yhlee@isslab.korea.ac.kr?part=1