Re: [PATCH] usb: gadget: f_ncm: Fix NPE in ncm_bind error path

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Sep 11, 2025 at 02:50:15PM +0800, Kuen-Han Tsai wrote:
> Hi Greg,
> 
> On Sat, Sep 6, 2025 at 8:15 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 04, 2025 at 07:46:13PM +0800, Kuen-Han Tsai wrote:
> > > When an ncm_bind/unbind cycle occurs, the ncm->notify_req pointer is
> > > left pointing to a stale address. If a subsequent call to ncm_bind()
> > > fails to allocate the endpoints, the function jumps to the unified error
> > > label. The cleanup code sees the stale ncm->notify_req pointer and calls
> > > usb_ep_free_request().
> > >
> > > This results in a NPE because it attempts to access the free_request
> > > function through the endpoint's operations table (ep->ops), which is
> > > NULL.
> > >
> > > Refactor the error path to use cascading goto labels, ensuring that
> > > resources are freed in reverse order of allocation. Besides, explicitly
> > > set pointers to NULL after freeing them.
> >
> > Why must the pointers be set to NULL?  What is checking and requiring
> > that?
> 
> I set them to null as a standard safety measure to prevent potential
> use-after-free issues. I can remove it if you prefer.

So either you have a use-after-free, or a NULL crash, either way it's
bad and the real bug should be fixed if this can happen.  If it can not
happen, then there is no need to set this to NULL.

> > And this unwinding is tailor-made for the guard() type of logic, why not
> > convert this code to do that instead, which will fix all of these bugs
> > automatically, right?
> 
> The __free() cleanup mechanism is unfortunately infeasible in this
> case. The usb_ep_free_request(ep, req) requires two parameters, but
> the automatic cleanup mechanism only needs one: the resource being
> freed.
> 
> Since the struct usb_request doesn't contain the pointer to its
> associated endpoint, the @free function cannot retrieve the ep pointer
> it needs for the cleanup call.  We would need to add an endpoint
> pointer to usb_request to make it work. However, this will be a
> significant change and we might also need to refactor drivers that use
> the usb_ep_free_request(ep, req), usb_ep_queue(ep, req) and
> usb_ep_dequeue(ep, req) as the endpoint parameter is no longer needed.

It's odd that the ep is needed to create a request, but it's not saved.
So yes, I think it should be saved, and that will make the cleanup logic
a lot simpler, as well as allow us to use the __free() logic overall.

> I also want to point out that this bug isn't specific to the f_ncm
> driver. The f_acm, f_rndis and f_ecm are also vulnerable because their
> bind paths have the same flaw. We have already observed this issue in
> both f_ncm and f_acm on Android devices.
> 
> My plan was to merge this fix for f_ncm first and then apply the same
> pattern to the other affected drivers. However, I am happy to have a
> more thorough design discussion if you feel using __free()/guard()
> automatic cleanup is the better path forward.

I think all of them need to be fixed up, and by adding the endpoint
pointer to the request, that should help make the logic overall for all
of these drivers simpler and easier to maintain over time.

So yes, if you could do that, it would be wonderful.

thanks,

greg k-h