drivers/usb/misc/yurex.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
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! */
Because t0 precedes t1 precedes t2 precedes t3, T1 can obtain the
file_operations pointer for the device (t1, while the minor is still
registered), then continue into yurex_open() where it calls
usb_get_intfdata() and gets NULL back, leading to a NULL dereference.
Fix the race by calling usb_deregister_dev() first, which removes the
device from usb_minors[] before the interface data pointer is cleared.
Any concurrent usb_open() that arrives after usb_deregister_dev()
returns will fail to look up the fops and will never reach yurex_open().
Reported-by: Junzhe Li <ginger.jzllee@gmail.com>
Closes: https://lore.kernel.org/linux-usb/2026042718-unwieldy-dicing-626f@gregkh
Signed-off-by: Junzhe Li <ginger.jzllee@gmail.com>
---
drivers/usb/misc/yurex.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index 7a482cdee1e9..136272ac24ba 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -310,11 +310,12 @@ static void yurex_disconnect(struct usb_interface *interface)
int minor = interface->minor;
dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
/* give back our minor */
usb_deregister_dev(interface, &yurex_class);
+ usb_set_intfdata(interface, NULL);
+
/* prevent more I/O from starting */
usb_poison_urb(dev->urb);
usb_poison_urb(dev->cntl_urb);
--
2.34.1On 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
On Thu, May 28, 2026 at 5:56 PM Oliver Neukum <oneukum@suse.com> 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
On 29.05.26 08:58, Ginger wrote: > 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. Hi, if I understand the logic correctly, the order in yurex_disconnect() makes sure if yurex_open() and yurex_disconnect() race, yurex_open() will never see an unregistered device with intfdata != NULL. That is, precisely because without a lock the race is unavoidable the newly opening task will be guaranteed to know that it has lost the race. Regards Oliver
On Fri, May 29, 2026 at 6:31 PM Oliver Neukum <oneukum@suse.com> wrote: > > On 29.05.26 08:58, Ginger wrote: > > > 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. > > Hi, > > if I understand the logic correctly, the order in yurex_disconnect() > makes sure if yurex_open() and yurex_disconnect() race, yurex_open() > will never see an unregistered device with intfdata != NULL. > That is, precisely because without a lock the race is unavoidable > the newly opening task will be guaranteed to know that it has > lost the race. > > Regards > Oliver > Hi, Yes, I believe we are on the same page about this patch. Regards, Junzhe
© 2016 - 2026 Red Hat, Inc.