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