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

[Mattijs Korpershoek]

On Thu, Nov 27, 2025 at 14:44, Petr Benes <petr.benes at ysoft.com> wrote:

> On 11/27/25 11:12 AM, Mattijs Korpershoek wrote:
>> Hi Michal,
>>
>> Thank you for the patch.
>>
>> On Tue, Nov 25, 2025 at 09:58, Michal Vokáč <michal.vokac at ysoft.com> wrote:
>>
>>> From: Petr Beneš <petr.benes at 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?
>
> Can't speak for for generic gadget stuff without preparation. In this
> case it is solely ci_udc who does the mess.
>
> It works with its own
>
> struct ci_ep {
>         struct usb_ep ep;
>         struct list_head queue;
>         bool req_primed;
>         const struct usb_endpoint_descriptor *desc;
> };
>
> Which are held in (struct ci_drv).ep[] and the problematic code is
> ci_udc specific.

Ok.

I have compared the changes with the linux driver
(drivers/usb/chipidea/udc.c) and see a couple of differences in error
handling

For example, in linux, in ep_disable(), we return -EBUSY instead of 0.

0 seems to tell the caller that we sucesfully disabled an ep but we did
not do anything since it was already disabled.

Can we do the same error handling as in the linux driver ?

>
>>
>>>
>>> Moreover, the patch gets rid of possible outstanding requests if the
>>> endpoint's state changes to disabled.
>>>
>>> Signed-off-by: Petr Beneš <petr.benes at ysoft.com>
>>> Signed-off-by: Michal Vokáč <michal.vokac at 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
>
>
> ________________________________
>
> CONFIDENTIALITY NOTICE
> This message is for the named person's use only. It may contain confidential, proprietary or legally privileged information.
> If you receive this message in error, please immediately delete it and all copies of it from your system, destroy any hard copies of it and notify us by email to info at ysoft.com with a copy of this message. You must not, directly or indirectly, use, disclose, distribute, print or copy any part of this message if you are not the intended recipient. Y Soft and any of its subsidiaries each reserves the right to monitor all e-mail communications through its networks.
> Y Soft is neither liable for the proper, complete transmission of the information contained in this communication nor any delay in its receipt. This email was scanned for the presence of computer viruses. In the unfortunate event of infection Y Soft does not accept liability.
> Any views expressed in this message are those of the individual sender, except where the message states otherwise and the sender is authorised to state them.