[PATCH v6 0/2] usb: gadget: f_tcm: fix a short write infoleak and a completion-path sleep

[PATCH v6 0/2] usb: gadget: f_tcm: fix a short write infoleak and a completion-path sleep

[Yehyeong Lee]

Two fixes in the f_tcm BOT completion path, both reachable from a remote
USB host.

Patch 1 is a sleep-in-atomic that came up reviewing v4:
bot_send_bad_status() and bot_send_status() queue requests with GFP_KERNEL
from endpoint completion callbacks, which run with interrupts disabled.  It
predates the infoleak fix, so it goes first.

Patch 2 is the short write data-out infoleak this series started as: a BOT
host that sends less than it declared leaves the command buffer tail
uninitialised and f_tcm commits the full declared length to the backing
store.  It also stops the status phase from draining ep_out once the OUT
data stage is over, which otherwise wedges the bulk pipe on a failed write.

v6:
- 2/2: skip the ep_out drain whenever the OUT data stage has completed, not
  only on a short packet, so a full write the backend rejects no longer
  wedges the pipe.
  v5: split the GFP_ATOMIC fix out as patch 1/2.
  v4: keep cmd->data_len so the CSW residue stays correct; skip the drain
   with a command flag (v2/v3 cleared data_len, zeroing the residue).
  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/

Yehyeong Lee (2):
  usb: gadget: f_tcm: queue the BOT status requests with GFP_ATOMIC
  usb: gadget: f_tcm: reject a short write data-out transfer

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


base-commit: 2c3418fffa9d037b2038a6db48be63f9e2291806
-- 
2.43.0

[PATCH v6 1/2] usb: gadget: f_tcm: queue the BOT status requests with GFP_ATOMIC

[Yehyeong Lee]

bot_send_bad_status() and bot_send_status() run from endpoint completion
callbacks - bot_send_bad_status() via usbg_data_write_cmpl()'s cleanup
path, and both of them via bot_read_compl() after a BOT read.  Those
callbacks are called with interrupts disabled and must not sleep, but both
functions queue the status or drain request with GFP_KERNEL, which can.

Queue with GFP_ATOMIC, like the other usb_ep_queue() calls reached from the
same callbacks (bot_enqueue_sense_code() and bot_err_compl()).

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>
---
 drivers/usb/gadget/function/f_tcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 9e6d4f39900a2..12e2b20e33b92 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -139,7 +139,7 @@ static void bot_send_bad_status(struct usbg_cmd *cmd)
 		req->complete = bot_err_compl;
 		req->context = cmd;
 		req->buf = fu->cmd[0].buf;
-		usb_ep_queue(ep, req, GFP_KERNEL);
+		usb_ep_queue(ep, req, GFP_ATOMIC);
 	} else {
 		bot_enqueue_sense_code(fu, cmd);
 	}
@@ -167,7 +167,7 @@ static int bot_send_status(struct usbg_cmd *cmd, bool moved_data)
 		csw->Status = US_BULK_STAT_OK;
 		fu->bot_status.req->context = cmd;
 
-		ret = usb_ep_queue(fu->ep_in, fu->bot_status.req, GFP_KERNEL);
+		ret = usb_ep_queue(fu->ep_in, fu->bot_status.req, GFP_ATOMIC);
 		if (ret)
 			pr_err("%s(%d) ERR: %d\n", __func__, __LINE__, ret);
 	} else {
-- 
2.43.0

Re: [PATCH v6 1/2] usb: gadget: f_tcm: queue the BOT status requests with GFP_ATOMIC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: f_tcm: command leak and session hang on usb_ep_queue failure
--

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

usb: gadget: f_tcm: queue the BOT status requests with GFP_ATOMIC

This commit changes the allocation flags used when queuing BOT status requests
in the f_tcm USB gadget function. Since the completion callbacks are executed
with interrupts disabled, GFP_KERNEL is replaced with GFP_ATOMIC to prevent
sleeping in an atomic context.

> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 9e6d4f39900a2..12e2b20e33b92 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -139,7 +139,7 @@ static void bot_send_bad_status(struct usbg_cmd *cmd)
>  		req->complete = bot_err_compl;
>  		req->context = cmd;
>  		req->buf = fu->cmd[0].buf;
> -		usb_ep_queue(ep, req, GFP_KERNEL);
> +		usb_ep_queue(ep, req, GFP_ATOMIC);

[Severity: High]
In bot_send_bad_status(), if usb_ep_queue() fails, do the target core command
(usbg_cmd) and dynamically allocated buffers (such as cmd->data_buf) leak here?
The return value is completely ignored, which means the completion callback
is never invoked to free them.

Since the target core expects all commands to complete before session teardown,
could this cause target_wait_for_sess_cmds() to loop forever waiting for
completion, resulting in a permanent kernel hang? A faulty or untrusted USB host
can trigger this by resetting the bus or disconnecting at the exact moment
usb_ep_queue() attempts to queue the status request.

>  	} else {
>  		bot_enqueue_sense_code(fu, cmd);
>  	}
> @@ -167,7 +167,7 @@ static int bot_send_status(struct usbg_cmd *cmd, bool moved_data)
>  		csw->Status = US_BULK_STAT_OK;
>  		fu->bot_status.req->context = cmd;
>  
> -		ret = usb_ep_queue(fu->ep_in, fu->bot_status.req, GFP_KERNEL);
> +		ret = usb_ep_queue(fu->ep_in, fu->bot_status.req, GFP_ATOMIC);
>  		if (ret)
>  			pr_err("%s(%d) ERR: %d\n", __func__, __LINE__, ret);

[Severity: High]
Similarly in bot_send_status(), the return value of usb_ep_queue() is checked
but only logged. If the queue fails, does this lead to the same command leak
and session hang because the status completion handler is never called to free
the command?

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

[PATCH v6 2/2] 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.  By the time usbg_data_write_cmpl() runs the OUT
data request has completed, so mark the command and skip the ep_out drain
in bot_send_bad_status() - there is nothing left on the wire, and draining
it re-arms ep_out for data the host already finished sending and wedges the
bulk pipe.  This mirrors how f_mass_storage stops draining once the host
ends the OUT stage.  A command rejected before its data stage still drains,
since the host has yet to send it.

A short 8192/4096 WRITE then commits nothing to the backing store instead
of 4096 bytes of stale heap, and no OUT drain is 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>
---
 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 12e2b20e33b92..a37a3ec8e7a4d 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) {
+	/* nothing to drain once the OUT data phase has completed */
+	if (cmd->data_len && !(cmd->flags & USBG_CMD_OUT_DATA_DONE)) {
 		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);
+	/* the OUT data request completed; nothing is left to drain */
+	cmd->flags |= USBG_CMD_OUT_DATA_DONE;
+
+	/* a short data-out left the tail 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;
 	}
 
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index e1d5a93916127..50764a33cdf36 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_DATA_DONE		BIT(1)
 
 	/* UAS only */
 	u16 tag;
-- 
2.43.0

Re: [PATCH v6 2/2] 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: info leak of uninit heap via dummy IN data
--

commit b6ddf000f25244005d71ddb597b63ddd0507d15b
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. A bot host can declare a larger out transfer than it
delivers and end the data stage with a short packet.

Fail 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 12e2b20e33b92..a37a3ec8e7a4d 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) {
> +	/* nothing to drain once the OUT data phase has completed */
> +	if (cmd->data_len && !(cmd->flags & USBG_CMD_OUT_DATA_DONE)) {
>  		if (cmd->is_read) {
>  			ep = fu->ep_in;
>  			req = fu->bot_req_in;

[Severity: High]
Does this code transmit uninitialized kernel heap memory to an untrusted
usb host?

Looking at bot_prepare_reqs(), fu->cmd[0].buf is allocated with kmalloc()
without being zeroed:

drivers/usb/gadget/function/f_tcm.c:bot_prepare_reqs() {
    ...
	fu->cmd[0].buf = kmalloc(fu->ep_out->maxpacket, GFP_KERNEL);
	if (!fu->cmd[0].buf)
		goto err_buf;
    ...
}

The usb host writes a 31-byte bot cbw into it, leaving the remaining space
(up to 993 bytes for superspeed endpoints) uninitialized.

When a data-in command (host read) is rejected prior to its data phase,
this uninitialized fu->cmd[0].buf is explicitly queued further down inside
this same bot_send_bad_status() block to satisfy the host's requested length:

		req->buf = fu->cmd[0].buf;
		usb_ep_queue(ep, req, GFP_ATOMIC);

The completion handler then continuously re-queues this same buffer until
the host's requested data length (which can be up to 4GB) is fully satisfied:

drivers/usb/gadget/function/f_tcm.c:bot_err_compl() {
    ...
	if (cmd->data_len) {
        ...
		usb_ep_queue(ep, req, GFP_ATOMIC);
		return;
	}
    ...
}

Can an attached usb host trigger this by sending an invalid data-in command,
creating a large information leak?

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