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

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

[Junzhe Li]

In ld_usb_disconnect(), usb_set_intfdata(interface, NULL) was called before
usb_deregister_dev(interface, &ld_usb_class).
This opens a race window with usb_open() in the USB core:

  T0 (ld_usb_disconnect)               T1 (usb_open)
  --------------------------           -------------------------
                                       fops = usb_minors[minor]  [t0]
                                       /* fops still valid here */
                                       file->f_op->open(inode, file)
                                         ld_usb_open()
                                           dev = usb_get_intfdata() [t1]
                                           if (!dev)
                                             return -ENODEV;
  usb_set_intfdata(iface, NULL) [t2]
                                           access dev->mutex [t3]
                                           /* dev is NULL! */
  usb_deregister_dev()
    usb_minors[minor] = NULL   [t4]

Because t0 precedes t1 precedes t2 precedes t3 precedes t4, T1 can obtain
the file_operations pointer for the device (t0, while the minor is still
registered), then continue into ld_usb_open() where it calls
usb_get_intfdata() and gets NULL back, leading to a NULL dereference.

The intuition is that the global exposure to the 'usb_minors' should be 
disabled first, so that subsequent nullification of the interface data 
pointer can be regarded as a local cleanup.

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 ld_usb_open().

Reported-by: Junzhe Li <ginger.jzllee@gmail.com>
Closes: https://lore.kernel.org/linux-usb/2026060157-pettiness-corporal-05eb@gregkh/
Signed-off-by: Junzhe Li <ginger.jzllee@gmail.com>
---
 drivers/usb/misc/ldusb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index c74f142f6637..ba65de2d4808 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -756,13 +756,14 @@ static void ld_usb_disconnect(struct usb_interface *intf)
 	int minor;
 
 	dev = usb_get_intfdata(intf);
-	usb_set_intfdata(intf, NULL);
 
 	minor = intf->minor;
 
 	/* give back our minor */
 	usb_deregister_dev(intf, &ld_usb_class);
 
+	usb_set_intfdata(intf, NULL);
+
 	usb_poison_urb(dev->interrupt_in_urb);
 	usb_poison_urb(dev->interrupt_out_urb);
 
-- 
2.34.1

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

[Oliver Neukum]

On 01.06.26 09:55, Junzhe Li wrote:
> In ld_usb_disconnect(), usb_set_intfdata(interface, NULL) was called before
> usb_deregister_dev(interface, &ld_usb_class).
> This opens a race window with usb_open() in the USB core:
> 
>    T0 (ld_usb_disconnect)               T1 (usb_open)
>    --------------------------           -------------------------
>                                         fops = usb_minors[minor]  [t0]
>                                         /* fops still valid here */
>                                         file->f_op->open(inode, file)
>                                           ld_usb_open()
>                                             dev = usb_get_intfdata() [t1]
>                                             if (!dev)
>                                               return -ENODEV;
>    usb_set_intfdata(iface, NULL) [t2]
>                                             access dev->mutex [t3]
>                                             /* dev is NULL! */

Again, no:

         dev = usb_get_intfdata(interface);

         if (!dev)
                 return -ENODEV;

ld_usb_open() checks for NULL

>    usb_deregister_dev()
>      usb_minors[minor] = NULL   [t4]
> 
> Because t0 precedes t1 precedes t2 precedes t3 precedes t4, T1 can obtain
> the file_operations pointer for the device (t0, while the minor is still
> registered), then continue into ld_usb_open() where it calls
> usb_get_intfdata() and gets NULL back, leading to a NULL dereference.
> 
> The intuition is that the global exposure to the 'usb_minors' should be
> disabled first, so that subsequent nullification of the interface data
> pointer can be regarded as a local cleanup.

No, these drivers use intfdata == NULL as a flag to indicate that
a device is in the process of going away. The order is disconnect,
together with the rwsem, makes sure that ld_usb_open either finishes
and sets the flag for a device being open, or gets intfdata == NULL
and returns -ENODEV.

The order should not be changed in these drivers.

	Sorry
		Oliver

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

[Greg KH]

On Mon, Jun 01, 2026 at 03:55:24PM +0800, Junzhe Li wrote:
> In ld_usb_disconnect(), usb_set_intfdata(interface, NULL) was called before
> usb_deregister_dev(interface, &ld_usb_class).
> This opens a race window with usb_open() in the USB core:
> 
>   T0 (ld_usb_disconnect)               T1 (usb_open)
>   --------------------------           -------------------------
>                                        fops = usb_minors[minor]  [t0]
>                                        /* fops still valid here */
>                                        file->f_op->open(inode, file)
>                                          ld_usb_open()
>                                            dev = usb_get_intfdata() [t1]
>                                            if (!dev)
>                                              return -ENODEV;
>   usb_set_intfdata(iface, NULL) [t2]
>                                            access dev->mutex [t3]
>                                            /* dev is NULL! */
>   usb_deregister_dev()
>     usb_minors[minor] = NULL   [t4]
> 
> Because t0 precedes t1 precedes t2 precedes t3 precedes t4, T1 can obtain
> the file_operations pointer for the device (t0, while the minor is still
> registered), then continue into ld_usb_open() where it calls
> usb_get_intfdata() and gets NULL back, leading to a NULL dereference.
> 
> The intuition is that the global exposure to the 'usb_minors' should be 
> disabled first, so that subsequent nullification of the interface data 
> pointer can be regarded as a local cleanup.
> 
> 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 ld_usb_open().
> 
> Reported-by: Junzhe Li <ginger.jzllee@gmail.com>

When you author a patch, no neeed for reported-by.

> Closes: https://lore.kernel.org/linux-usb/2026060157-pettiness-corporal-05eb@gregkh/

No, I said if this is actually an issue, send a patch, not that this is
an issue :)

> Signed-off-by: Junzhe Li <ginger.jzllee@gmail.com>

What commit id does this "fix"?

thanks,

greg k-h