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

[sashiko-bot@kernel.org]

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