[PATCH v7 0/3] usb: gadget: f_tcm: fix infoleak, sleep, and UAF in the BOT completion path

[PATCH v7 0/3] usb: gadget: f_tcm: fix infoleak, sleep, and UAF in the BOT completion path

[Yehyeong Lee]

Three fixes in the f_tcm BOT completion path, all reachable from a remote
USB host.

Patch 1 queues the status requests with GFP_ATOMIC: bot_send_bad_status()
and bot_send_status() run from endpoint completion callbacks, which are
called with interrupts disabled.

Patch 2 rejects a short write data-out transfer, which otherwise commits
the uninitialised tail of the bounce buffer to the backing store, and stops
the status phase from draining ep_out once the OUT data stage is over.

Patch 3 clears the scatter list when the BOT status request is reused, so
an sg-capable UDC no longer walks the freed scatterlist of the previous
command.

v7:
- New 3/3: clear req->num_sgs/req->sg in bot_send_bad_status() to fix a
  use-after-free of the previous command's scatter list.
  v6: 2/2 skips the ep_out drain whenever the OUT data stage has completed,
   not only on a short packet.
  v5: split the GFP_ATOMIC fix out as its own patch.
  v4: keep cmd->data_len so the CSW residue stays correct; skip the drain
   with a command flag.
  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 (3):
  usb: gadget: f_tcm: queue the BOT status requests with GFP_ATOMIC
  usb: gadget: f_tcm: reject a short write data-out transfer
  usb: gadget: f_tcm: clear the scatter list when reusing the BOT status
    request

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


base-commit: 2c3418fffa9d037b2038a6db48be63f9e2291806
-- 
2.43.0

[PATCH v7 1/3] 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 v7 1/3] 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: unhandled usb_ep_queue() failure leaks TCM command
--

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

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

This commit fixes sleeping in atomic context by changing the queue flags
in bot_send_bad_status() and bot_send_status() from GFP_KERNEL to
GFP_ATOMIC.

> 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]
Does this code leak the TCM command and hang the USB BOT session if
usb_ep_queue() fails?

By changing the allocation flag to GFP_ATOMIC, usb_ep_queue() is more
likely to fail under memory pressure.

Because the return value isn't checked here, if it fails, the endpoint
completion callback (bot_err_compl) will never execute. The callback is
responsible for queuing the next CBW (Command Block Wrapper) to receive
subsequent commands, so the BOT instance will stall.

The command is also not freed by transport_generic_free_cmd(), leading
to eventual session tag exhaustion. A host-initiated USB Bulk Reset
(US_BULK_RESET_REQUEST) resumes listening for new CBWs but does not
explicitly abort the leaked TCM command.

>  	} 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]
Does this code leak the TCM command and hang the USB BOT session if
usb_ep_queue() fails?

Similarly, if the GFP_ATOMIC allocation fails under memory pressure, the
error is only logged and we unconditionally return 0.

The endpoint completion callback will never execute, which stalls the
BOT session because the next CBW won't be queued. The command is
permanently leaked.

>  	} else {
>  		cmd->csw_code = US_BULK_STAT_FAIL;
>  		bot_send_bad_status(cmd);

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

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

[Yehyeong Lee]

The ignored usb_ep_queue() return value is pre-existing - it has been that
way since the original c52661d60f63, and this patch only corrects the
allocation flag.  GFP_ATOMIC is required here because bot_send_bad_status()
and bot_send_status() run from the endpoint completion callbacks with
interrupts disabled; GFP_KERNEL was the bug.

Handling a usb_ep_queue() failure (completing the command so the session
does not stall) is a separate, pre-existing issue and would be a follow-up
rather than part of this flag fix.

Thanks,
Yehyeong

[PATCH v7 2/3] 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 v7 2/3] usb: gadget: f_tcm: reject a short write data-out transfer

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v7 3/3] usb: gadget: f_tcm: clear the scatter list when reusing the BOT status request

[Yehyeong Lee]

bot_send_bad_status() reuses fu->bot_req_in/bot_req_out to send the CSW or
drain the data phase, but a preceding sg command left req->sg and
req->num_sgs pointing at that command's t_data_sg - set in
bot_send_read_response() for a read, or usbg_prepare_w_request() for a
write. That command is freed before the status request runs, so an
sg-capable UDC walks a freed scatter list in dma_map_sg(): it reads freed
heap out to the host on an IN endpoint, or writes host data into a freed
page on an OUT endpoint.

Clear req->num_sgs and req->sg so the UDC uses the linear req->buf instead.

With KASAN, a read command followed by a rejected command that reuses
bot_req_in reports a slab-use-after-free in the UDC's scatterlist walk,
with the previous command's sgl_free() on the freeing stack; clearing the
scatter list removes it, while a normal sg read still queues its own
scatterlist.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index a37a3ec8e7a4d..e727e8e1cfa0c 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -140,6 +140,9 @@ static void bot_send_bad_status(struct usbg_cmd *cmd)
 		req->complete = bot_err_compl;
 		req->context = cmd;
 		req->buf = fu->cmd[0].buf;
+		/* the status request is reused; drop the previous command's sg list */
+		req->num_sgs = 0;
+		req->sg = NULL;
 		usb_ep_queue(ep, req, GFP_ATOMIC);
 	} else {
 		bot_enqueue_sense_code(fu, cmd);
-- 
2.43.0

Re: [PATCH v7 3/3] usb: gadget: f_tcm: clear the scatter list when reusing the BOT status request

[sashiko-bot]

> bot_send_bad_status() reuses fu->bot_req_in/bot_req_out to send the CSW or
> drain the data phase, but a preceding sg command left req->sg and
> req->num_sgs pointing at that command's t_data_sg - set in
> bot_send_read_response() for a read, or usbg_prepare_w_request() for a
> write. That command is freed before the status request runs, so an
> sg-capable UDC walks a freed scatter list in dma_map_sg(): it reads freed
> heap out to the host on an IN endpoint, or writes host data into a freed
> page on an OUT endpoint.
> 
> Clear req->num_sgs and req->sg so the UDC uses the linear req->buf instead.
> 
> With KASAN, a read command followed by a rejected command that reuses
> bot_req_in reports a slab-use-after-free in the UDC's scatterlist walk,
> with the previous command's sgl_free() on the freeing stack; clearing the
> scatter list removes it, while a normal sg read still queues its own
> scatterlist.
> 
> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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