usb: tegra-xudc: check ep->desc before dereferencing
Subsystems (2)
PATCH 1/1 usb: tegra-xudc: check ep->desc before dereferencing
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)
--- drivers/usb/gadget/udc/tegra-xudc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/usb/gadget/udc/tegra-xudc.c +6 -3
@@ -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,
Signature · 3 lines
Alexey V. Vissarionov gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net
Jon Hunter
maintainerQuoted 17 lines · Alexey V. Vissarionov
Check
ep->descbefore dereferencing it integra_xudc_req_done()call and later in this functiontegra_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 <[email protected]>--- drivers/usb/gadget/udc/tegra-xudc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/usb/gadget/udc/tegra-xudc.c +0
-0
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2667
Quoted 7 lines
drivers/usb/gadget/udc/tegra-xudc.c +4
-0@@ -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
Signature · 1 line
nvpublic
Good ${greeting_time}!
Quoted 8 lines · Jon Hunter
Quoted 8 lines
Check
ep->descbefore dereferencing it integra_xudc_req_done()drivers/usb/gadget/udc/tegra-xudc.c +0
-0@@ -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);
Quoted 9 lines
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2667
Quoted 5 lines
drivers/usb/gadget/udc/tegra-xudc.c +1
-0+/* tegra_xudc_req_done() dereferences ep->desc; check it here */
drivers/usb/gadget/udc/tegra-xudc.c +3
-0+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.
Signature · 3 lines
Alexey V. Vissarionov gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net
PATCH 1/1 usb: tegra-xudc: check ep and ep->desc before deref
In reply to Jon HunterCheck 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)
--- drivers/usb/gadget/udc/tegra-xudc.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
drivers/usb/gadget/udc/tegra-xudc.c +16 -3
@@ -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,
Signature · 3 lines
Alexey V. Vissarionov gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net
Jon Hunter
maintainerRe: [PATCH v1] usb: tegra-xudc: check ep and ep->desc before deref
Quoted 17 lines · Alexey V. Vissarionov
Check ep before dereferencing it in
trb_phys_to_virt()andep->descbefore dereferencing it integra_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 <[email protected]>--- drivers/usb/gadget/udc/tegra-xudc.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)drivers/usb/gadget/udc/tegra-xudc.c +0
-0
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2663
Quoted 6 lines
drivers/usb/gadget/udc/tegra-xudc.c +3
-0@@ -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) {drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2664
Quoted 1 line
drivers/usb/gadget/udc/tegra-xudc.c +1
-0+dev_err (xudc->dev,"Unbelievable: ep is NULL\n" );
I quite like the 'Unbelievable' but 'unexpected NULL pointer for ep' is also fine.
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2676
Quoted 12 lines
drivers/usb/gadget/udc/tegra-xudc.c +9
-0+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
Signature · 1 line
nvpublic
Re: [PATCH v1] usb: tegra-xudc: check ep and ep->desc before deref
Good ${greeting_time}!
Quoted 6 lines · Jon Hunter
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2663
Quoted 3 lines
drivers/usb/gadget/udc/tegra-xudc.c +3
-0+/* 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.
Quoted 3 lines
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2664
Quoted 1 line
drivers/usb/gadget/udc/tegra-xudc.c +1
-0+dev_err (xudc->dev,"Unbelievable: ep is NULL\n" );I quite like the 'Unbelievable' but 'unexpected NULL pointer for ep' is also fine.
ACK.
Quoted 6 lines
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2672
Quoted 3 lines
(quoted diff) +1
-0+ /* tegra_xudc_req_done() dereferences ep->desc; check ithere */
drivers/usb/gadget/udc/tegra-xudc.c +1
-0+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.
Signature · 3 lines
Alexey V. Vissarionov gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net
PATCH v2 1/1 usb: tegra-xudc: check ep and ep->desc before deref
In reply to Jon HunterCheck 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)
--- drivers/usb/gadget/udc/tegra-xudc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/usb/gadget/udc/tegra-xudc.c +14 -3
@@ -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,
Signature · 3 lines
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
Quoted 17 lines · Alexey V. Vissarionov
Check ep before dereferencing it in
trb_phys_to_virt()andep->descbefore dereferencing it integra_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 <[email protected]>--- drivers/usb/gadget/udc/tegra-xudc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/usb/gadget/udc/tegra-xudc.c +0
-0
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2665
Quoted 8 lines
drivers/usb/gadget/udc/tegra-xudc.c +5
-0@@ -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.
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2674
Quoted 9 lines
drivers/usb/gadget/udc/tegra-xudc.c +6
-0+ 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
Good ${greeting_time}!
Quoted 8 lines · Alan Stern
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2665
Quoted 5 lines
drivers/usb/gadget/udc/tegra-xudc.c +5
-0+/* 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.
Quoted 2 lines
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.
Quoted 3 lines
Of course, if it is reasonable for ep or
ep->descto sometimes be NULL, then the checks should be made. But if that were true, I don't know why you would calldev_err().
This was suggested by Jon Hunter on 16 Apr 2025 08:43:58 +0100 and I've agreed that would be wise.
Signature · 3 lines
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
Quoted 14 lines · Alexey V. Vissarionov
Good ${greeting_time}!
Quoted 8 lines · Alan Stern
drivers/usb/gadget/udc/tegra-xudc.c static void tegra_xudc_handle_transfer_completion(struct tegra_xudc *xudc, · line 2665
Quoted 5 lines
drivers/usb/gadget/udc/tegra-xudc.c +5
-0+/* 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?
Quoted 2 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
Of course, if it is reasonable for ep or
ep->descto sometimes be NULL, then the checks should be made. But if that were true, I don't know why you would calldev_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