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

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

[Yehyeong Lee]

usbg_data_write_cmpl() copies se_cmd->data_length bytes out of the bounce
buffer into the command's scatter-gather list without checking how much the
host actually sent.  A BOT host can declare a larger OUT transfer than it
delivers and end the data stage with a short packet: req->actual is then
shorter than req->length, but the completion handler still commits the full
declared length.  The tail past req->actual is whatever was already in the
kmalloc'd bounce buffer, which is not zeroed, so uninitialised kernel heap
is written to the backing store and can be read back by the host.

Fail the command when req->actual does not match req->length instead of
committing the stale tail.  The short packet means the host has already
ended the OUT stage, so mark the command and report the residue in the CSW
without re-arming ep_out to drain data that will never arrive, the way
f_mass_storage treats a short_packet_received.

A short 8192/4096 WRITE then commits nothing to the backing store instead
of 4096 bytes of stale heap, and the CSW reports a residue of 8192 with no
OUT drain queued.

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>
---
v4:
- Keep cmd->data_len so the CSW still reports the untransferred length as
  the residue.  v2/v3 cleared it, which reported a residue of 0 and told
  the host the whole transfer had been accepted.  Skip the ep_out drain
  with a command flag instead, matching f_mass_storage.
  v3: https://lore.kernel.org/linux-usb/20261013015416.2503787-1-yhlee@isslab.korea.ac.kr/
  v2: https://lore.kernel.org/linux-usb/20261013005854.2470384-1-yhlee@isslab.korea.ac.kr/
  v1: https://lore.kernel.org/linux-usb/20261012154422.2436134-1-yhlee@isslab.korea.ac.kr/

 drivers/usb/gadget/function/f_tcm.c | 14 +++++++++++---
 drivers/usb/gadget/function/tcm.h   |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 9e6d4f39900a2..d686fbcf36f72 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -120,7 +120,8 @@ static void bot_send_bad_status(struct usbg_cmd *cmd)
 
 	csw->Residue = cpu_to_le32(cmd->data_len);
 
-	if (cmd->data_len) {
+	/* a short data-out already ended the phase; draining it would hang */
+	if (cmd->data_len && !(cmd->flags & USBG_CMD_OUT_SHORT)) {
 		if (cmd->is_read) {
 			ep = fu->ep_in;
 			req = fu->bot_req_in;
@@ -1090,8 +1091,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);
+	/* 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);
+
+		if (!req->status)
+			cmd->flags |= USBG_CMD_OUT_SHORT;
+
+		dev_err_ratelimited(&gadget->dev, "%s() state %d transfer failed\n",
+				    __func__, cmd->state);
 		goto cleanup;
 	}
 
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index e1d5a93916127..1f9f7181603f3 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -88,6 +88,7 @@ struct usbg_cmd {
 
 	u32 flags;
 #define USBG_CMD_PENDING_DATA_WRITE	BIT(0)
+#define USBG_CMD_OUT_SHORT		BIT(1)
 
 	/* UAS only */
 	u16 tag;

base-commit: 2c3418fffa9d037b2038a6db48be63f9e2291806
-- 
2.43.0

Re: [PATCH v4] 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: sleep in atomic context via GFP_KERNEL
--

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

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

usbg_data_write_cmpl() copies se_cmd->data_length bytes out of the bounce
buffer into the command's scatter-gather list without checking how much the
host actually sent. This patch fails the command when req->actual does not
match req->length instead of committing the stale tail.

> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 9e6d4f39900a2..d686fbcf36f72 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -120,7 +120,8 @@ static void bot_send_bad_status(struct usbg_cmd *cmd)
>  
>  	csw->Residue = cpu_to_le32(cmd->data_len);
>  
> -	if (cmd->data_len) {
> +	/* a short data-out already ended the phase; draining it would hang */
> +	if (cmd->data_len && !(cmd->flags & USBG_CMD_OUT_SHORT)) {
>  		if (cmd->is_read) {
>  			ep = fu->ep_in;
>  			req = fu->bot_req_in;
[ ... ]
> @@ -1090,8 +1091,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);
> +	/* 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);
> +
> +		if (!req->status)
> +			cmd->flags |= USBG_CMD_OUT_SHORT;
> +
> +		dev_err_ratelimited(&gadget->dev, "%s() state %d transfer failed\n",
> +				    __func__, cmd->state);
>  		goto cleanup;

[Severity: High]
Does this code sleep in atomic context?

usbg_data_write_cmpl() is a USB endpoint completion callback running in atomic
context.

When the transfer fails, or a short packet occurs, this path jumps to the
cleanup label, which synchronously calls:

    transport_send_check_condition_and_sense(se_cmd,
                                             TCM_CHECK_CONDITION_ABORT_CMD, 0);

This invokes the queue_status callback which eventually calls
bot_send_bad_status(). As seen earlier in the file, bot_send_bad_status()
can submit the request with GFP_KERNEL:

drivers/usb/gadget/function/f_tcm.c:bot_send_bad_status() {
    ...
    if (cmd->data_len && !(cmd->flags & USBG_CMD_OUT_SHORT)) {
        ...
        usb_ep_queue(ep, req, GFP_KERNEL);
    ...
}

If req->status is non-zero, USBG_CMD_OUT_SHORT is not set, so the above path
is taken. Will passing GFP_KERNEL to usb_ep_queue() in this atomic completion
path cause a sleep in atomic context?

>  	}
>  
>  	if (req->num_sgs == 0) {

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