[PATCH] usb: tegra-xudc: check ep->desc before dereferencing

[PATCH] usb: tegra-xudc: check ep->desc before dereferencing

[Alexey V. Vissarionov]

Check ep->desc before dereferencing it in tegra_xudc_req_done() call
and later in this function tegra_xudc_handle_transfer_completion()

Found by ALT Linux Team (altlinux.org) and Linux Verification Center
(linuxtesting.org)

Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
---
 drivers/usb/gadget/udc/tegra-xudc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index c7fdbc55fb0b97ed..0322e984e2c6fd91 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -2661,6 +2661,10 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 	trb = trb_phys_to_virt(ep, trb_read_data_ptr(event));
 	req = trb_to_request(ep, trb);
 
+	/* tegra_xudc_req_done() dereferences ep->desc; check it here */
+	if (!ep || !ep->desc)
+		return;
+
 	/*
 	 * TDs are complete on short packet or when the completed TRB is the
 	 * last TRB in the TD (the CHAIN bit is unset).
@@ -2678,7 +2682,7 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 
 		tegra_xudc_req_done(ep, req, 0);
 
-		if (ep->desc && usb_endpoint_xfer_control(ep->desc))
+		if (usb_endpoint_xfer_control(ep->desc))
 			tegra_xudc_ep0_req_done(xudc);
 
 		/*
@@ -2694,8 +2698,7 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 		dev_warn(xudc->dev, "transfer event on dequeued request\n");
 	}
 
-	if (ep->desc)
-		tegra_xudc_ep_kick_queue(ep);
+	tegra_xudc_ep_kick_queue(ep);
 }
 
 static void tegra_xudc_handle_transfer_event(struct tegra_xudc *xudc,


-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

Re: [PATCH] usb: tegra-xudc: check ep->desc before dereferencing

[Jon Hunter]

On 15/04/2025 18:42, Alexey V. Vissarionov wrote:
> Check ep->desc before dereferencing it in tegra_xudc_req_done() call
> and later in this function tegra_xudc_handle_transfer_completion()
> 
> Found by ALT Linux Team (altlinux.org) and Linux Verification Center
> (linuxtesting.org)
> 
> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
> ---
>   drivers/usb/gadget/udc/tegra-xudc.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> index c7fdbc55fb0b97ed..0322e984e2c6fd91 100644
> --- a/drivers/usb/gadget/udc/tegra-xudc.c
> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> @@ -2661,6 +2661,10 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
>   	trb = trb_phys_to_virt(ep, trb_read_data_ptr(event));
>   	req = trb_to_request(ep, trb);
>   
> +	/* tegra_xudc_req_done() dereferences ep->desc; check it here */
> +	if (!ep || !ep->desc)
> +		return;
> +

Looking at the code, it would seem that we should check !ep at the start 
of the function, because it has already been used at this point. Also
!ep is worthy of an error message because that should never happen.

Cheers
Jon

-- 
nvpublic

Re: [PATCH] usb: tegra-xudc: check ep->desc before dereferencing

[Alexey V. Vissarionov]

Good ${greeting_time}!

On 2025-04-16 08:43:58 +0100, Jon Hunter wrote:

 >> Check ep->desc before dereferencing it in tegra_xudc_req_done()
 >> --- a/drivers/usb/gadget/udc/tegra-xudc.c
 >> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
 >> @@ -2661,6 +2661,10 @@ static void
 >> tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 >>	trb = trb_phys_to_virt(ep, trb_read_data_ptr(event));
 >>	req = trb_to_request(ep, trb);
 >>
 >> +	/* tegra_xudc_req_done() dereferences ep->desc; check it
 >> here */
 >> +	if (!ep || !ep->desc)
 >> +		return;
 >> +
 > Looking at the code, it would seem that we should check !ep at
 > the start of the function, because it has already been used at
 > this point. Also !ep is worthy of an error message because that
 > should never happen.

Agree, the check should be performed before trb_phys_to_virt() and,
possibly, there should be two separate checks.

Any suggestions on a error message?
  dev_err(xudc->dev, "Unbelievable: ep is NULL\n");
(and same for ep->desc) looks ok for me. Or s/Unbelieva/Impossi/ ?

Next version of the patch follows.


-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

[PATCH v1] usb: tegra-xudc: check ep and ep->desc before deref

[Alexey V. Vissarionov]

Check ep before dereferencing it in trb_phys_to_virt() and ep->desc
before dereferencing it in tegra_xudc_req_done()

Found by ALT Linux Team (altlinux.org) and Linux Verification Center
(linuxtesting.org)

Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
---
 drivers/usb/gadget/udc/tegra-xudc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index c7fdbc55fb0b97ed..cae99ebe9f85868d 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -2658,9 +2658,23 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 	short_packet = (trb_read_cmpl_code(event) ==
 			TRB_CMPL_CODE_SHORT_PACKET);
 
+	/* trb_phys_to_virt() dereferences ep; check it here */
+	if (!ep)
+	{
+		dev_err(xudc->dev, "Unbelievable: ep is NULL\n");
+		return;
+	}
+
 	trb = trb_phys_to_virt(ep, trb_read_data_ptr(event));
 	req = trb_to_request(ep, trb);
 
+	/* tegra_xudc_req_done() dereferences ep->desc; check it here */
+	if (!ep->desc)
+	{
+		dev_err(xudc->dev, "Unbelievable: ep->desc is NULL\n");
+		return;
+	}
+
 	/*
 	 * TDs are complete on short packet or when the completed TRB is the
 	 * last TRB in the TD (the CHAIN bit is unset).
@@ -2678,7 +2692,7 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 
 		tegra_xudc_req_done(ep, req, 0);
 
-		if (ep->desc && usb_endpoint_xfer_control(ep->desc))
+		if (usb_endpoint_xfer_control(ep->desc))
 			tegra_xudc_ep0_req_done(xudc);
 
 		/*
@@ -2694,8 +2708,7 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 		dev_warn(xudc->dev, "transfer event on dequeued request\n");
 	}
 
-	if (ep->desc)
-		tegra_xudc_ep_kick_queue(ep);
+	tegra_xudc_ep_kick_queue(ep);
 }
 
 static void tegra_xudc_handle_transfer_event(struct tegra_xudc *xudc,


-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

Re: [PATCH v1] usb: tegra-xudc: check ep and ep->desc before deref

[Jon Hunter]

On 16/04/2025 10:55, Alexey V. Vissarionov wrote:
> Check ep before dereferencing it in trb_phys_to_virt() and ep->desc
> before dereferencing it in tegra_xudc_req_done()
> 
> Found by ALT Linux Team (altlinux.org) and Linux Verification Center
> (linuxtesting.org)
> 
> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
> ---
>   drivers/usb/gadget/udc/tegra-xudc.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> index c7fdbc55fb0b97ed..cae99ebe9f85868d 100644
> --- a/drivers/usb/gadget/udc/tegra-xudc.c
> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> @@ -2658,9 +2658,23 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
>   	short_packet = (trb_read_cmpl_code(event) ==
>   			TRB_CMPL_CODE_SHORT_PACKET);
>   
> +	/* trb_phys_to_virt() dereferences ep; check it here */
> +	if (!ep)
> +	{

Please make sure you run 'checkpatch.pl' as I am sure if will flag that 
the above should be ...

     if (!ep) {

> +		dev_err(xudc->dev, "Unbelievable: ep is NULL\n");

I quite like the 'Unbelievable' but 'unexpected NULL pointer for ep' is 
also fine.

> +		return;
> +	}
> +
>   	trb = trb_phys_to_virt(ep, trb_read_data_ptr(event));
>   	req = trb_to_request(ep, trb);
>   
> +	/* tegra_xudc_req_done() dereferences ep->desc; check it here */
> +	if (!ep->desc)
> +	{
> +		dev_err(xudc->dev, "Unbelievable: ep->desc is NULL\n");
> +		return;
> +	}

I am not sure about the error message here, because the existing code 
just skips this. So it is not clear if this can happen and could be 
expected.

Jon

-- 
nvpublic

Re: [PATCH v1] usb: tegra-xudc: check ep and ep->desc before deref

[Alexey V. Vissarionov]

Good ${greeting_time}!

On 2025-04-16 11:20:10 +0100, Jon Hunter wrote:

 >> +	/* trb_phys_to_virt() dereferences ep; check it here */
 >> +	if (!ep)
 >> +	{
 > Please make sure you run 'checkpatch.pl' as I am sure if will
 > flag that the above should be ...
 > if (!ep) {

ACK.

 >> +		dev_err(xudc->dev, "Unbelievable: ep is NULL\n");
 > I quite like the 'Unbelievable' but 'unexpected NULL pointer
 > for ep' is also fine.

ACK.

 >> +	/* tegra_xudc_req_done() dereferences ep->desc; check it
 >> here */
 >> +	if (!ep->desc)
 > I am not sure about the error message here, because the existing
 > code just skips this. So it is not clear if this can happen and
 > could be expected.

Now ep->desc is checked immediately after tegra_xudc_req_done(),
where it is dereferenced, so it was somehow expected. My suggestion
is just to check this expection earlier.

Next version of the patch follows.


-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

[PATCH v2] usb: tegra-xudc: check ep and ep->desc before deref

[Alexey V. Vissarionov]

Check ep before dereferencing it in trb_phys_to_virt() and ep->desc
before dereferencing it in tegra_xudc_req_done()

Found by ALT Linux Team (altlinux.org) and Linux Verification Center
(linuxtesting.org)

Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
---
 drivers/usb/gadget/udc/tegra-xudc.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index c7fdbc55fb0b97ed..d61a0675e18f448f 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -2658,9 +2658,21 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 	short_packet = (trb_read_cmpl_code(event) ==
 			TRB_CMPL_CODE_SHORT_PACKET);
 
+	/* trb_phys_to_virt() dereferences ep; check it here */
+	if (!ep) {
+		dev_err(xudc->dev, "unexpected NULL pointer: ep\n");
+		return;
+	}
+
 	trb = trb_phys_to_virt(ep, trb_read_data_ptr(event));
 	req = trb_to_request(ep, trb);
 
+	/* tegra_xudc_req_done() dereferences ep->desc; check it here */
+	if (!ep->desc) {
+		dev_err(xudc->dev, "unexpected NULL pointer: ep->desc\n");
+		return;
+	}
+
 	/*
 	 * TDs are complete on short packet or when the completed TRB is the
 	 * last TRB in the TD (the CHAIN bit is unset).
@@ -2678,7 +2690,7 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 
 		tegra_xudc_req_done(ep, req, 0);
 
-		if (ep->desc && usb_endpoint_xfer_control(ep->desc))
+		if (usb_endpoint_xfer_control(ep->desc))
 			tegra_xudc_ep0_req_done(xudc);
 
 		/*
@@ -2694,8 +2706,7 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
 		dev_warn(xudc->dev, "transfer event on dequeued request\n");
 	}
 
-	if (ep->desc)
-		tegra_xudc_ep_kick_queue(ep);
+	tegra_xudc_ep_kick_queue(ep);
 }
 
 static void tegra_xudc_handle_transfer_event(struct tegra_xudc *xudc,



-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

Re: [PATCH v2] usb: tegra-xudc: check ep and ep->desc before deref

[Alan Stern]

On Wed, Apr 16, 2025 at 03:00:00PM +0300, Alexey V. Vissarionov wrote:
> Check ep before dereferencing it in trb_phys_to_virt() and ep->desc
> before dereferencing it in tegra_xudc_req_done()
> 
> Found by ALT Linux Team (altlinux.org) and Linux Verification Center
> (linuxtesting.org)
> 
> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
> ---
>  drivers/usb/gadget/udc/tegra-xudc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> index c7fdbc55fb0b97ed..d61a0675e18f448f 100644
> --- a/drivers/usb/gadget/udc/tegra-xudc.c
> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> @@ -2658,9 +2658,21 @@ static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc,
>  	short_packet = (trb_read_cmpl_code(event) ==
>  			TRB_CMPL_CODE_SHORT_PACKET);
>  
> +	/* trb_phys_to_virt() dereferences ep; check it here */
> +	if (!ep) {
> +		dev_err(xudc->dev, "unexpected NULL pointer: ep\n");
> +		return;
> +	}

Is this condition something that is totally under the kernel's control?  
That is, is ep always passed in by a driver and there's never a valid 
reason for it to be NULL?

Then there's really no need for this check.  In real life it will never 
trigger.  And if it does, because of a programming bug, you're better 
off getting the stack dump that comes with a NULL-pointer dereference -- 
it would certainly be a lot more visible to the developer when testing 
new code than a easy-to-miss error message, and it would indicate where 
the actual problem originated.

> +
>  	trb = trb_phys_to_virt(ep, trb_read_data_ptr(event));
>  	req = trb_to_request(ep, trb);
>  
> +	/* tegra_xudc_req_done() dereferences ep->desc; check it here */
> +	if (!ep->desc) {
> +		dev_err(xudc->dev, "unexpected NULL pointer: ep->desc\n");
> +		return;
> +	}

Same here.

Of course, if it is reasonable for ep or ep->desc to sometimes be NULL, 
then the checks should be made.  But if that were true, I don't know why 
you would call dev_err().

Alan Stern

Re: [PATCH v2] usb: tegra-xudc: check ep and ep->desc before deref

[Alexey V. Vissarionov]

Good ${greeting_time}!

On 2025-04-16 10:13:05 -0400, Alan Stern wrote:


 >> +	/* trb_phys_to_virt() dereferences ep; check it here */
 >> +	if (!ep) {
 >> +		dev_err(xudc->dev, "unexpected NULL pointer: ep\n");
 >> +		return;
 >> +	}
 > Is this condition something that is totally under the kernel's
 > control? That is, is ep always passed in by a driver and there's
 > never a valid reason for it to be NULL?

IIUC, the endpoints are reported by the device. But the device
may be something like STM32 uC with malicious firmware.

 > Then there's really no need for this check. In real life it
 > will never trigger.

With real devices. But ready-to-use STM32F103C8T6 boards are sold
for only 10...15 CNY, so one would need only to write a firmware
and to flash it in the board using 20 CNY program-and-debug tool.

 > Of course, if it is reasonable for ep or ep->desc to sometimes
 > be NULL, then the checks should be made. But if that were true,
 > I don't know why you would call dev_err().

This was suggested by Jon Hunter on 16 Apr 2025 08:43:58 +0100 and
I've agreed that would be wise.


-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

Re: [PATCH v2] usb: tegra-xudc: check ep and ep->desc before deref

[Alan Stern]

On Mon, Apr 21, 2025 at 06:07:28PM +0300, Alexey V. Vissarionov wrote:
> Good ${greeting_time}!
> 
> On 2025-04-16 10:13:05 -0400, Alan Stern wrote:
> 
> 
>  >> +	/* trb_phys_to_virt() dereferences ep; check it here */
>  >> +	if (!ep) {
>  >> +		dev_err(xudc->dev, "unexpected NULL pointer: ep\n");
>  >> +		return;
>  >> +	}
>  > Is this condition something that is totally under the kernel's
>  > control? That is, is ep always passed in by a driver and there's
>  > never a valid reason for it to be NULL?
> 
> IIUC, the endpoints are reported by the device. But the device
> may be something like STM32 uC with malicious firmware.

That doesn't matter, because the code you are changing doesn't run on 
the host.  It runs on the gadget.  If a gadget with malicious firmware 
crashes, who cares?  It would be a good thing, not a bad thing.

Besides, the condition you are testing for, namely !ep, can never happen 
anyway.  Here's why:

This routine -- tegra_xudc_handle_transfer_completion() -- is called 
from tegra_xudc_handle_transfer_event() in the case where comp_code is 
TRB_CMPL_CODE_SUCCESS or TRB_CMPL_CODE_SHORT_PACKET.  Either way, 
comp_code will be different from TRB_CMPL_CODE_BABBLE_DETECTED_ERR, so 
the code immediately preceding the function call will already have 
dereferenced ep.  If ep is NULL, the system will crash before your new 
code is executed.

>  > Then there's really no need for this check. In real life it
>  > will never trigger.
> 
> With real devices. But ready-to-use STM32F103C8T6 boards are sold
> for only 10...15 CNY, so one would need only to write a firmware
> and to flash it in the board using 20 CNY program-and-debug tool.

Do those boards run a Linux kernel?  If they don't then they won't run 
the code you are changing, so your statement is irrelevant.

>  > Of course, if it is reasonable for ep or ep->desc to sometimes
>  > be NULL, then the checks should be made. But if that were true,
>  > I don't know why you would call dev_err().
> 
> This was suggested by Jon Hunter on 16 Apr 2025 08:43:58 +0100 and
> I've agreed that would be wise.

The more I look at this, the more this change seems unnecessary.

Alan Stern