Re: [PATCH v2] usb: gadget: f_uvc: fix NULL pointer dereference during unbind race

[Jimmy Hu (xWF)]

On Wed, Mar 11, 2026 at 8:46 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Mar 09, 2026 at 01:31:07PM +0800, Jimmy Hu wrote:
> > Commit b81ac4395bbe ("usb: gadget: uvc: allow for application to cleanly
> > shutdown") introduced two stages of synchronization waits totaling 1500ms
> > in uvc_function_unbind() to prevent several types of kernel panics.
> > However, this timing-based approach is insufficient during power
> > management (PM) transitions.
> >
> > When the PM subsystem starts freezing user space processes, the
> > wait_event_interruptible_timeout() is aborted early, which allows the
> > unbind thread to proceed and nullify the gadget pointer
> > (cdev->gadget = NULL):
> >
> > [  814.123447][  T947] configfs-gadget.g1 gadget.0: uvc: uvc_function_unbind()
> > [  814.178583][ T3173] PM: suspend entry (deep)
> > [  814.192487][ T3173] Freezing user space processes
> > [  814.197668][  T947] configfs-gadget.g1 gadget.0: uvc: uvc_function_unbind no clean disconnect, wait for release
> >
> > When the PM subsystem resumes or aborts the suspend and tasks are
> > restarted, the V4L2 release path is executed and attempts to access the
> > already nullified gadget pointer, triggering a kernel panic:
> >
> > [  814.292597][    C0] PM: pm_system_irq_wakeup: 479 triggered dhdpcie_host_wake
> > [  814.386727][ T3173] Restarting tasks ...
> > [  814.403522][ T4558] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
> > [  814.404021][ T4558] pc : usb_gadget_deactivate+0x14/0xf4
> > [  814.404031][ T4558] lr : usb_function_deactivate+0x54/0x94
> > [  814.404078][ T4558] Call trace:
> > [  814.404080][ T4558]  usb_gadget_deactivate+0x14/0xf4
> > [  814.404083][ T4558]  usb_function_deactivate+0x54/0x94
> > [  814.404087][ T4558]  uvc_function_disconnect+0x1c/0x5c
> > [  814.404092][ T4558]  uvc_v4l2_release+0x44/0xac
> > [  814.404095][ T4558]  v4l2_release+0xcc/0x130
> >
> > This patch introduces the following safeguards:
> >
> > 1. State Synchronization (flag + mutex)
> > Introduced a 'func_unbound' flag in struct uvc_device. This allows
> > uvc_function_disconnect() to safely skip accessing the nullified
> > cdev->gadget pointer. As suggested by Alan Stern, this flag is protected
> > by a new mutex (uvc->lock) to ensure proper memory ordering and prevent
> > instruction reordering or speculative loads.
> >
> > 2. Lifecycle Management (kref)
> > Introduced kref to manage the struct uvc_device lifecycle. This prevents
> > Use-After-Free (UAF) by ensuring the structure is only freed after the
> > final reference, including the V4L2 release path, is dropped.
> >
> > Fixes: b81ac4395bbe ("usb: gadget: uvc: allow for application to cleanly shutdown")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Jimmy Hu <hhhuuu@xxxxxxxxxx>
> > ---
> > v1 -> v2:
> > - Renamed 'func_unbinding' to 'func_unbound' for clearer state semantics.
> > - Added a mutex (uvc->lock) to protect the 'func_unbound' flag and ensure
> >   proper memory ordering, as suggested by Alan Stern.
> > - Integrated kref to manage the struct uvc_device lifecycle, allowing the
> >   removal of redundant buffer cleanup skip logic in uvc_v4l2_disable().
> >
> > v1: https://patchwork.kernel.org/project/linux-usb/patch/20260224083955.1375032-1-hhhuuu@xxxxxxxxxx/
> >
> >  drivers/usb/gadget/function/f_uvc.c    | 27 +++++++++++++++++++++++++-
> >  drivers/usb/gadget/function/uvc.h      |  4 ++++
> >  drivers/usb/gadget/function/uvc_v4l2.c |  2 ++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > index 494fdbc4e85b..485cd91770d5 100644
> > --- a/drivers/usb/gadget/function/f_uvc.c
> > +++ b/drivers/usb/gadget/function/f_uvc.c
> > @@ -413,8 +413,17 @@ uvc_function_disconnect(struct uvc_device *uvc)
> >  {
> >       int ret;
> >
> > +     mutex_lock(&uvc->lock);
> > +     if (uvc->func_unbound) {
> > +             pr_info("uvc: unbound, skipping function deactivate\n");
>
> When drivers work properly, they are quiet.  Also, why are you not using
> uvcg_info() here, this pr_info() gives no device specific information so
> you know what device this is happening to.
>
>
>
> > +             goto unlock;
> > +     }
> > +
> >       if ((ret = usb_function_deactivate(&uvc->func)) < 0)
> >               uvcg_info(&uvc->func, "UVC disconnect failed with %d\n", ret);
> > +
> > +unlock:
> > +     mutex_unlock(&uvc->lock);
> >  }
> >
> >  /* --------------------------------------------------------------------------
> > @@ -659,6 +668,9 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> >       int ret = -EINVAL;
> >
> >       uvcg_info(f, "%s()\n", __func__);
> > +     mutex_lock(&uvc->lock);
> > +     uvc->func_unbound = false;
> > +     mutex_unlock(&uvc->lock);
> >
> >       opts = fi_to_f_uvc_opts(f->fi);
> >       /* Sanity check the streaming endpoint module parameters. */
> > @@ -974,6 +986,13 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> >       return &opts->func_inst;
> >  }
> >
> > +void uvc_device_release(struct kref *kref)
> > +{
> > +     struct uvc_device *uvc = container_of(kref, struct uvc_device, kref);
> > +
> > +     kfree(uvc);
> > +}
> > +
> >  static void uvc_free(struct usb_function *f)
> >  {
> >       struct uvc_device *uvc = to_uvc(f);
> > @@ -982,7 +1001,7 @@ static void uvc_free(struct usb_function *f)
> >       if (!opts->header)
> >               config_item_put(&uvc->header->item);
> >       --opts->refcnt;
> > -     kfree(uvc);
> > +     kref_put(&uvc->kref, uvc_device_release);
> >  }
> >
> >  static void uvc_function_unbind(struct usb_configuration *c,
> > @@ -994,6 +1013,9 @@ static void uvc_function_unbind(struct usb_configuration *c,
> >       long wait_ret = 1;
> >
> >       uvcg_info(f, "%s()\n", __func__);
> > +     mutex_lock(&uvc->lock);
> > +     uvc->func_unbound = true;
> > +     mutex_unlock(&uvc->lock);
> >
> >       kthread_cancel_work_sync(&video->hw_submit);
> >
> > @@ -1046,8 +1068,11 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
> >       if (uvc == NULL)
> >               return ERR_PTR(-ENOMEM);
> >
> > +     kref_init(&uvc->kref);
> > +     mutex_init(&uvc->lock);
> >       mutex_init(&uvc->video.mutex);
> >       uvc->state = UVC_STATE_DISCONNECTED;
> > +     uvc->func_unbound = true;
> >       init_waitqueue_head(&uvc->func_connected_queue);
> >       opts = fi_to_f_uvc_opts(fi);
> >
> > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> > index 676419a04976..22b70f25607d 100644
> > --- a/drivers/usb/gadget/function/uvc.h
> > +++ b/drivers/usb/gadget/function/uvc.h
> > @@ -155,6 +155,9 @@ struct uvc_device {
> >       enum uvc_state state;
> >       struct usb_function func;
> >       struct uvc_video video;
> > +     struct kref kref;
>
> But there is already a device reference count in the vdev structure,
> right?  Are you now having 2 reference counts for the same device?
> That's going to cause a lot of problems.
>
> > +     struct mutex lock;
>
> Please document what this lock is locking.
>
> thanks,
>
> greg k-h

Hi Greg, Alan,

I have just sent out v3 of this patch, which addresses the feedback regarding
redundant reference counting and log noise. Specifically, I've replaced
kref with a completion, switched to pr_debug() with device-specific context
to avoid NULL dereferences, and added the requested mutex documentation.

Link to v3: https://lore.kernel.org/all/20260319084640.478695-1-hhhuuu@xxxxxxxxxx/

Thanks!
Jimmy