Re: [PATCH REPOST] usb: udc: Make tracing aware that usb_ep_alloc_request() can return NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: use-after-free in usb_ep_queue() and usb_ep_dequeue() tracepoints
--

commit 1373e903abc2252ba5340f5231a6fb6306db9019
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

usb: udc: Make tracing aware that usb_ep_alloc_request() can return NULL

This commit updates the udc_log_req trace event class to handle NULL
request pointers by checking the pointer before accessing its fields. This
prevents a NULL pointer dereference when usb_ep_alloc_request() fails and
returns NULL.

Link: https://lore.kernel.org/all/6aab0160.e91c2013.3bdb06.0005.GAE@google.com/

> diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
> index fa3e6ddf0a128..05533eb1bae0c 100644
> --- a/drivers/usb/gadget/udc/trace.h
> +++ b/drivers/usb/gadget/udc/trace.h
> @@ -239,15 +239,15 @@ DECLARE_EVENT_CLASS(udc_log_req,
>  	),
>  	TP_fast_assign(
>  		__assign_str(name);
> -		__entry->length = req->length;
> -		__entry->actual = req->actual;
> -		__entry->num_sgs = req->num_sgs;
> -		__entry->num_mapped_sgs = req->num_mapped_sgs;
> -		__entry->stream_id = req->stream_id;
> -		__entry->no_interrupt = req->no_interrupt;
> -		__entry->zero = req->zero;
> -		__entry->short_not_ok = req->short_not_ok;
> -		__entry->status = req->status;
> +		__entry->length = req ? req->length : 0;
> +		__entry->actual = req ? req->actual : 0;

[Severity: High]
While this successfully avoids a dereference when req is NULL, does this
still leave a use-after-free when tracing usb_ep_queue() or
usb_ep_dequeue()?

If a gadget driver submits a request via usb_ep_queue(), the UDC driver can
complete it synchronously and return ownership to the gadget driver. For
instance, in dummy_queue():

    usb_gadget_giveback_request(_ep, _req);

If the gadget driver's completion handler then frees the request, control
eventually returns to usb_ep_queue() which invokes the tracepoint:

    ret = ep->ops->queue(ep, req, gfp_flags);
out:
    trace_usb_ep_queue(ep, req, ret);

Since trace_usb_ep_queue() shares this udc_log_req event class, won't req
point to freed memory, causing a use-after-free when evaluating
req->length? A similar scenario can occur in usb_ep_dequeue().

> +		__entry->num_sgs = req ? req->num_sgs : 0;
> +		__entry->num_mapped_sgs = req ? req->num_mapped_sgs : 0;
> +		__entry->stream_id = req ? req->stream_id : 0;
> +		__entry->no_interrupt = req ? req->no_interrupt : 0;
> +		__entry->zero = req ? req->zero : 0;
> +		__entry->short_not_ok = req ? req->short_not_ok : 0;
> +		__entry->status = req ? req->status : 0;
>  		__entry->ret = ret;
>  		__entry->req = req;
>  	),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012094911.wHPc_9lJ@linutronix.de?part=1