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

[sashiko-bot@kernel.org]

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