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

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

[Michal Vokáč]

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.

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

[PATCH 2/2] usb: ci_udc: cosmetics: EP and requests debug info

[Michal Vokáč]

From: Petr Beneš <petr.benes@ysoft.com>

Make a note in an unexpected situation, e.g. queuing a request
on a disabled endpoint, enabling an enabled endpoint...

Signed-off-by: Petr Beneš <petr.benes@ysoft.com>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 drivers/usb/gadget/ci_udc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index b3bbbb6ad32c..ea8939060efd 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -273,8 +273,10 @@ ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags)
 	if (ci_ep->desc)
 		num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 
-	if (num == 0 && controller.ep0_req)
+	if (num == 0 && controller.ep0_req) {
+		DBG("%s: already got controller.ep0_req = %p\n", __func__, controller.ep0_req);
 		return &controller.ep0_req->req;
+	}
 
 	ci_req = calloc(1, sizeof(*ci_req));
 	if (!ci_req)
@@ -296,6 +298,8 @@ static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *req)
 
 	if (ci_ep->desc)
 		num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
+	else
+		DBG("%s: no endpoint %p descriptor\n", __func__, ci_ep);
 
 	if (num == 0) {
 		if (!controller.ep0_req)
@@ -622,8 +626,10 @@ static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 			break;
 	}
 
-	if (&ci_req->req != _req)
+	if (&ci_req->req != _req) {
+		DBG("%s: ci_req not found in the queue\n", __func__);
 		return -EINVAL;
+	}
 
 	list_del_init(&ci_req->queue);
 
-- 
2.43.0

Re: [PATCH 2/2] usb: ci_udc: cosmetics: EP and requests debug info

[Mattijs Korpershoek]

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>
>
> Make a note in an unexpected situation, e.g. queuing a request
> on a disabled endpoint, enabling an enabled endpoint...
>
> Signed-off-by: Petr Beneš <petr.benes@ysoft.com>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>  drivers/usb/gadget/ci_udc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>

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

[Marek Vasut]

On 11/25/25 9:58 AM, Michal Vokáč 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.

Is there a test case which triggers this problem ?

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

[Petr Benes]

On 11/25/25 1:26 PM, Marek Vasut wrote:
> [EXTERNAL EMAIL]
>
> On 11/25/25 9:58 AM, Michal Vokáč 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.
>
> Is there a test case which triggers this problem ?

e.g. repetitive use of tftp, the first works, second may hang, resets
the system, whatever

________________________________

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

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

[Mattijs Korpershoek]

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

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

[Petr Benes]

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

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.

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


________________________________

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

Re: [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@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@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?
>
> 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@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
>
>
> ________________________________
>
> 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@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.

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

[Petr Benes]

On 11/28/25 9:42 AM, Mattijs Korpershoek wrote:
> On Thu, Nov 27, 2025 at 14:44, Petr Benes <petr.benes@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@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?
>>
>> 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.

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

Sure. -EBUSY seems to used mainly by *_register_driver(), I'll test it
and return with the updated patch.>
>>
>>>
>>>>
>>>> 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



________________________________

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

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

[Mattijs Korpershoek]

On Mon, Dec 01, 2025 at 09:20, Petr Benes <petr.benes@ysoft.com> wrote:
>>>
>>> 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.
>
> Fair.>
>> Can we do the same error handling as in the linux driver ?
>
> Sure. -EBUSY seems to used mainly by *_register_driver(), I'll test it
> and return with the updated patch.>

Thank you. Please also make sure to look at ep_enable() in linux as a
similar pattern is found in this U-Boot patch.
(we return 0 instead of a negative error code)

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