Re: [PATCH AUTOSEL 6.18-5.10] usb: fsl_qe_udc: check qe_alloc_request() failure in ch9getstatus()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: fsl_qe_udc: sleeping allocation in atomic context in ch9getstatus()
- [Medium] usb: fsl_qe_udc: memory leak in ch9getstatus() when __qe_ep_queue() fails
--

commit 130ef9b6cbb58e7a139f83239ccf232ff79a3793
Author: Haoxiang Li <haoxiang_li2024@163.com>

usb: fsl_qe_udc: check qe_alloc_request() failure in ch9getstatus()

This commit adds a NULL check for the return value of qe_alloc_request()
in ch9getstatus() before passing it to container_of() and dereferencing
it, stalling the endpoint on allocation failure.

Link: https://patch.msgid.link/20260620120631.2894977-1-haoxiang_li2024@163.com

> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c b/drivers/usb/gadget/udc/fsl_qe_udc.c
> index aacfde06387c0..7911dca2135cf 100644
> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
> @@ -1945,6 +1945,7 @@ static void ch9getstatus(struct qe_udc *udc, u8 request_type, u16 value,
>  			u16 index, u16 length)
>  {
>  	u16 usb_status = 0;
> +	struct usb_request *usb_req;
>  	struct qe_req *req;
>  	struct qe_ep *ep;
>  	int status = 0;
> @@ -1983,8 +1984,11 @@ static void ch9getstatus(struct qe_udc *udc, u8 request_type, u16 value,
>  		}
>  	}
>  
> -	req = container_of(qe_alloc_request(&ep->ep, GFP_KERNEL),
> -					struct qe_req, req);
> +	usb_req = qe_alloc_request(&ep->ep, GFP_KERNEL);

[Severity: High]
Should this allocation in ch9getstatus() use GFP_ATOMIC instead of
GFP_KERNEL?

When a USB_REQ_GET_STATUS control request is received, ch9getstatus() is
called from the hardware interrupt handler while holding udc->lock with
interrupts disabled:

  qe_udc_irq()
    spin_lock_irqsave(&udc->lock, flags)
    rx_irq()
      qe_ep0_rx()
        ep0_setup_handle()
          setup_received_handle()
            ch9getstatus()
              qe_alloc_request(&ep->ep, GFP_KERNEL)
                kzalloc(sizeof(*req), gfp_flags)

Passing GFP_KERNEL here allows kzalloc() to sleep in atomic context.

> +	if (!usb_req)
> +		goto stall;
> +
> +	req = container_of(usb_req, struct qe_req, req);
>  	req->req.length = 2;
>  	req->req.buf = udc->statusbuf;
>  	*(u16 *)req->req.buf = cpu_to_le16(usb_status);

[Severity: Medium]
Can this path in ch9getstatus() also leak req if __qe_ep_queue() fails?

Just below this hunk, ch9getstatus() queues the allocated request:

drivers/usb/gadget/udc/fsl_qe_udc.c:ch9getstatus() {
	...
	status = __qe_ep_queue(&ep->ep, &req->req);

	if (status == 0)
		return;
stall:
	dev_err(udc->dev, "Can't respond to getstatus request \n");
	qe_ep0_stall(udc);
}

In __qe_ep_queue(), if !udc->driver or udc->gadget.speed == USB_SPEED_UNKNOWN,
it returns -ESHUTDOWN before adding req->queue to ep->queue, so
ownercomplete() will never be called to free req:

drivers/usb/gadget/udc/fsl_qe_udc.c:__qe_ep_queue() {
	...
	if (!udc->driver || udc->gadget.speed == USB_SPEED_UNKNOWN)
		return -ESHUTDOWN;
	...
}

When __qe_ep_queue() returns a non-zero status, ch9getstatus() falls
through to stall: without calling qe_free_request(&ep->ep, usb_req) or
kfree(req).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-95-sashal@kernel.org?part=1