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

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

[Petr Beneš]

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

v2: return values aligned with Linux as requested by Mattijs

 drivers/usb/gadget/ci_udc.c | 47 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 4bff75da759..c8953d48723 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 -EBUSY;
+	}
+
 	ci_ep->desc = desc;
 	ep->desc = desc;
 
@@ -385,19 +412,32 @@ 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__);
+		err = -EBUSY;
+		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;
+	err = 0;
+
+nodesc:
 	ep->desc = NULL;
 	ci_ep->req_primed = false;
-	return 0;
+	return err;
 }
 
 static int ci_bounce(struct ci_req *ci_req, int in)
@@ -606,6 +646,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.52.0

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

[Petr Beneš]

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

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
Signed-off-by: Petr Beneš <petr.benes@ysoft.com>
---

v2: no changes

 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 c8953d48723..046bb335ecb 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)
@@ -624,8 +628,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.52.0

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

[Mattijs Korpershoek]

Hi Petr,

Thank you for the patch.

On Thu, Dec 18, 2025 at 15:27, Petr Beneš <petr.benes@ysoft.com> wrote:

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

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

> ---
>
> v2: return values aligned with Linux as requested by Mattijs
>

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

[Mattijs Korpershoek]

Hi,

On Thu, 18 Dec 2025 15:27:35 +0100, Petr Beneš wrote:
> 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.
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/2] usb: ci_udc: Check ci_ep->desc before use
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/6a92e9827650797b6b5290621c1831fe32d6ea4b
[2/2] usb: ci_udc: cosmetics: EP and requests debug info
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/8ea70d8132df30c37c9592952c6267c2e9b4e562

--
Mattijs