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

[PATCH v3] 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
once this completion runs, so clear cmd->data_len unconditionally here:
otherwise a later BOT CHECK CONDITION - from the short write, or from any
WRITE that finishes its data phase and then fails in the backend - takes it
as outstanding data and queues an 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>
---
v3: clear cmd->data_len unconditionally once the data phase is over, not
    only on the short-write path, so a WRITE that finishes its data phase
    and then fails in the backend does not wedge the bulk pipe either.
v2: fail short data-out and clear cmd->data_len; rate-limit the message.

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

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 9e6d4f39900a2..cf6a394c4196b 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1090,8 +1090,15 @@ 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);
+	/* the OUT data phase is over; nothing left for the status phase to drain */
+	cmd->data_len = 0;
+
+	/* 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);
+
+		dev_err_ratelimited(&gadget->dev, "%s() state %d transfer failed\n",
+				    __func__, cmd->state);
 		goto cleanup;
 	}
 
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: gadget: f_tcm: UAF in usbg_data_write_cmpl due to early kref put
- [Medium] usb: gadget: f_tcm: incorrect CSW residue on short OUT transfers
--

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

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

This commit updates usbg_data_write_cmpl() to reject short data-out
transfers by verifying that req->actual matches req->length. It also
unconditionally clears cmd->data_len to prevent trailing OUT requests
from wedging the bulk pipe.

Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Cc: stable@vger.kernel.org

> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 9e6d4f39900a2..cf6a394c4196b 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1090,8 +1090,15 @@ 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);
> +	/* the OUT data phase is over; nothing left for the status phase to drain */
> +	cmd->data_len = 0;

[Severity: Medium]
Will clearing cmd->data_len here cause an incorrect BOT CSW residue to be
reported on short OUT transfers?

Looking at bot_send_bad_status(), it uses cmd->data_len to populate the CSW
residue field:

csw->Residue = cpu_to_le32(cmd->data_len);

If a short transfer occurs and usbg_data_write_cmpl() sets cmd->data_len to 0,
the device might incorrectly report a CSW residue of 0, hiding the fact that
data was dropped.

This appears to violate the USB Mass Storage BOT specification, which requires
reporting the exact difference between expected and actual data length for short
transfers.

> +
> +	/* 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);
> +
> +		dev_err_ratelimited(&gadget->dev, "%s() state %d transfer failed\n",
> +				    __func__, cmd->state);
>  		goto cleanup;

[Severity: High]
Can jumping to the cleanup block lead to a use-after-free or a double-put of
the se_cmd?

The cleanup block unconditionally drops the fabric reference and then
immediately accesses se_cmd:

cleanup:
	target_put_sess_cmd(se_cmd);
	...
	transport_send_check_condition_and_sense(se_cmd,
			TCM_CHECK_CONDITION_ABORT_CMD, 0);

Calling transport_send_check_condition_and_sense() after dropping the reference
via target_put_sess_cmd() appears to be a use-after-free.

Furthermore, if this error path is reached because the target core aborted the
command (which completes the USB request with an error like -ECONNRESET), the
target core will have already forcefully dropped both the ACK reference and the
execution reference via target_handle_abort().

When the dequeued request enters usbg_data_write_cmpl(), calling
target_put_sess_cmd() again could cause a double-put underflow and
dereference the freed se_cmd.

>  	}

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