[lvc-project] [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