Re: [PATCH 1/2] usb: ci_udc: Check ci_ep->desc before use

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Michal,

Thank you for the patch.

On Tue, Nov 25, 2025 at 09:58, Michal Vokáč <michal.vokac@ysoft.com> wrote:

> From: Petr Beneš <petr.benes@ysoft.com>
>
> There are two places where ci_ep->desc could be accessed despite it is
> not valid at that moment. Either the endpoint has not been enabled yet
> or it has been disabled meanwhile (The ethernet gadged behaves this way
> at least.). That results in dereferencing a null pointer.

I wonder if this is not a generic problem (that ep->desc) is reused.

Looking at usb_ep_enable() in include/linux/usb/gadget.h:

"""
static inline int usb_ep_enable(struct usb_ep *ep,
				const struct usb_endpoint_descriptor *desc)
{
	int ret;

	if (ep->enabled)
		return 0;

	ret = ep->ops->enable(ep, desc);
	if (ret)
		return ret;
"""

Can you please explain why the generic code is not enough to handle
this?

>
> Moreover, the patch gets rid of possible outstanding requests if the
> endpoint's state changes to disabled.
>
> Signed-off-by: Petr Beneš <petr.benes@ysoft.com>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  drivers/usb/gadget/ci_udc.c | 43 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index 4bff75da759d..b3bbbb6ad32c 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -308,6 +308,27 @@ static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *req)
>  	free(ci_req);
>  }
>  
> +static void request_complete(struct usb_ep *ep, struct ci_req *req, int status)
> +{
> +	if (req->req.status == -EINPROGRESS)
> +		req->req.status = status;
> +
> +	DBG("%s: req %p complete: status %d, actual %u\n",
> +	    ep->name, req, req->req.status, req->req.actual);
> +
> +	req->req.complete(ep, &req->req);
> +}
> +
> +static void request_complete_list(struct usb_ep *ep, struct list_head *list, int status)
> +{
> +	struct ci_req *req, *tmp_req;
> +
> +	list_for_each_entry_safe(req, tmp_req, list, queue) {
> +		list_del_init(&req->queue);
> +		request_complete(ep, req, status);
> +	}
> +}
> +
>  static void ep_enable(int num, int in, int maxpacket)
>  {
>  	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
> @@ -335,6 +356,12 @@ static int ci_ep_enable(struct usb_ep *ep,
>  	int num, in;
>  	num = desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
>  	in = (desc->bEndpointAddress & USB_DIR_IN) != 0;
> +
> +	if (ci_ep->desc) {
> +		DBG("%s: endpoint num %d in %d already enabled\n", __func__, num, in);
> +		return 0;
> +	}
> +
>  	ci_ep->desc = desc;
>  	ep->desc = desc;
>  
> @@ -385,16 +412,27 @@ static int ep_disable(int num, int in)
>  static int ci_ep_disable(struct usb_ep *ep)
>  {
>  	struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
> +	LIST_HEAD(req_list);
>  	int num, in, err;
>  
> +	if (!ci_ep->desc) {
> +		DBG("%s: attempt to disable a not enabled yet endpoint\n", __func__);
> +		goto nodesc;
> +	}
> +
>  	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
>  	in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
>  
> +	list_splice_init(&ci_ep->queue, &req_list);
> +	request_complete_list(ep, &req_list, -ESHUTDOWN);
> +
>  	err = ep_disable(num, in);
>  	if (err)
>  		return err;
>  
>  	ci_ep->desc = NULL;
> +
> +nodesc:
>  	ep->desc = NULL;
>  	ci_ep->req_primed = false;
>  	return 0;
> @@ -606,6 +644,11 @@ static int ci_ep_queue(struct usb_ep *ep,
>  	int in, ret;
>  	int __maybe_unused num;
>  
> +	if (!ci_ep->desc) {
> +		DBG("%s: ci_ep->desc == NULL, nothing to do!\n", __func__);
> +		return -EINVAL;
> +	}
> +
>  	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
>  	in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
>  
> -- 
> 2.43.0