Re: [PATCH] usb: misc: yurex: fix ordering of usb_deregister_dev() and usb_set_intfdata()

[Ginger]

On Thu, May 28, 2026 at 5:56 PM Oliver Neukum <oneukum@xxxxxxxx> wrote:
>
>
>
> On 28.05.26 10:27, Junzhe Li wrote:
> > In yurex_disconnect(), usb_set_intfdata(interface, NULL) was called
> > before usb_deregister_dev(interface, &yurex_class).
> > This opens a race window with usb_open() in the USB core:
> >
> >    T0 (yurex_disconnect)               T1 (usb_open)
> >    --------------------------           -------------------------
> >    usb_set_intfdata(iface, NULL) [t0]
> >                                         fops = usb_minors[minor]  [t1]
> >                                         /* fops still valid here */
> >    usb_deregister_dev()
> >      usb_minors[minor] = NULL   [t2]
> >                                         file->f_op->open(inode, file)
> >                                           yurex_open()
> >                                             dev = usb_get_intfdata() [t3]
> >                                             /* dev is NULL! */
>
> Yes, but yurex_open() checks for dev == NULL
> Could you please elaborate?
>
>         Regards
>                 Oliver
>

Hi Oliver,

Thanks for pointing that out. IMHO, the check for 'dev == NULL' does
not nullify this bug. The potential race window with the null value
check is elaborated below:

  T0 (yurex_disconnect)               T1 (usb_open)
  --------------------------           -------------------------
                                       fops = usb_minors[minor]  [t0]
                                       /* fops still valid here */

                                       file->f_op->open(inode, file)
                                         yurex_open()
                                           dev = usb_get_intfdata() [t1]
                                           if (!dev)
                                            return -ENODEV;
  usb_set_intfdata(iface, NULL) [t2]
                                           kref_get(&dev->kref); [t3]
                                           /* dev is NULL! */
  usb_deregister_dev()
    usb_minors[minor] = NULL   [t4]
    /* The global usb_minors is nullified here */

I think the intuition is that the global exposure (i.e., the
'usb_minors') of usb fops should be disabled first, so that the
subsequent nullification of internal fields can be considered local to
prevent concurrent accesses.

Please correct me if I understand incorrectly.

Regards,
Junzhe