mmc: vub300: fix NULL-deref and UAF on disconnect

PATCH In review Johan Hovold 2 participants 11 messages Mar 27 – Mar 31

Subsystems (2)

Stable branch MMC, SD and SDIO

lore

Share & tools

lore

Johan Hovold

author

2026-03-27 10:52 UTC #0 lore raw

cover mmc: vub300: fix NULL-deref and UAF on disconnect

This series fixes lifetime issues that can lead to NULL pointer dereferences and use-after-free.

Included are also two related cleanups.

Johan

Johan Hovold (4):
mmc: vub300: fix NULL-deref on disconnect
mmc: vub300: fix use-after-free on disconnect
mmc: vub300: rename probe error labels
mmc: vub300: clean up module init

 drivers/mmc/host/vub300.c | 70 +++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 32 deletions(-)

Signature · 1 line

2.52.0

Johan Hovold

author

2026-03-27 10:52 UTC #1 lore raw

PATCH 1/4 mmc: vub300: fix NULL-deref on disconnect

Make sure to deregister the controller before dropping the reference to the driver data on disconnect to avoid NULL-pointer dereferences or use-after-free.

Fixes 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")

Cc [email protected] # 3.0

Cc Tony Olech <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/mmc/host/vub300.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/host/vub300.c +1 -1

index ff49d0770506..f173c7cf4e1a 100644
@@ -2365,8 +2365,8 @@ static void vub300_disconnect(struct usb_interface *interface)
 			usb_set_intfdata(interface, NULL);
 			/* prevent more I/O from starting */
 			vub300->interface = NULL;
-			kref_put(&vub300->kref, vub300_delete);
 			mmc_remove_host(mmc);
+			kref_put(&vub300->kref, vub300_delete);
 			pr_info("USB vub300 remote SDIO host controller[%d]"
 				" now disconnected", ifnum);
 			return;

Signature · 1 line

2.52.0

Johan Hovold

author

2026-03-31 10:32 UTC #3 lore raw

Re: [PATCH 1/4] mmc: vub300: fix NULL-deref on disconnect

Quoted 14 lines · Ulf Hansson

Quoted 13 lines · Johan Hovold
Make sure to deregister the controller before dropping the reference to the driver data on disconnect to avoid NULL-pointer dereferences or use-after-free.
Fixes 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
Cc [email protected] # 3.0
Cc Tony Olech <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/mmc/host/vub300.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 14 lines

drivers/mmc/host/vub300.c static void vub300_disconnect(struct usb_interface *interface) · line 2369

Quoted 11 lines

drivers/mmc/host/vub300.c +1 -1

index ff49d0770506..f173c7cf4e1a 100644
@@ -2365,8 +2365,8 @@ static void vub300_disconnect(struct usb_interface *interface)
                        usb_set_intfdata(interface, NULL);
                        /* prevent more I/O from starting */
                        vub300->interface = NULL;
-                       kref_put(&vub300->kref, vub300_delete);
                        mmc_remove_host(mmc);
+                       kref_put(&vub300->kref, vub300_delete);

While this seems like a step in the right direction, I don't see why calling usb_set_intfdata(interface, NULL)

The interface data is only used in the USB bus callbacks and is not needed after disconnect().

Quoted 5 lines

and assigning
vub300->interface = NULL is safe.
For example, some of the workqueues might be running a work that uses the vub300->interface, isn't that a problem too?

The driver uses this pointer to indicate that the device has been disconnected. That doesn't mean that the implementation is correct (e.g. the check in vub300_pollwork_thread() should use some locking) but that would be pre-existing issues.

Johan

Ulf Hansson

2026-03-31 11:04 UTC #4 lore raw

Re: [PATCH 1/4] mmc: vub300: fix NULL-deref on disconnect

Quoted 37 lines · Johan Hovold

Quoted 14 lines · Ulf Hansson
Quoted 13 lines · Johan Hovold
Make sure to deregister the controller before dropping the reference to the driver data on disconnect to avoid NULL-pointer dereferences or use-after-free.
Fixes 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
Cc [email protected] # 3.0
Cc Tony Olech <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/mmc/host/vub300.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
drivers/mmc/host/vub300.c static void vub300_disconnect(struct usb_interface *interface) · line 2369
Quoted 11 lines
drivers/mmc/host/vub300.c +1 -1
index ff49d0770506..f173c7cf4e1a 100644
@@ -2365,8 +2365,8 @@ static void vub300_disconnect(struct usb_interface *interface)
                        usb_set_intfdata(interface, NULL);
                        /* prevent more I/O from starting */
                        vub300->interface = NULL;
-                       kref_put(&vub300->kref, vub300_delete);
                        mmc_remove_host(mmc);
+                       kref_put(&vub300->kref, vub300_delete);
While this seems like a step in the right direction, I don't see why calling usb_set_intfdata(interface, NULL)
The interface data is only used in the USB bus callbacks and is not needed after disconnect().
Quoted 3 lines
and assigning
vub300->interface = NULL is safe.

Quoted 7 lines

Quoted 2 lines
For example, some of the workqueues might be running a work that uses the vub300->interface, isn't that a problem too?
The driver uses this pointer to indicate that the device has been disconnected. That doesn't mean that the implementation is correct (e.g. the check in vub300_pollwork_thread() should use some locking) but that would be pre-existing issues.

Right, that was my thinking as well.

Out of curiosity, are you planning on fixing these issues too or is that left for later?

Kind regards
Uffe

Johan Hovold

author

2026-03-31 11:16 UTC #5 lore raw

Re: [PATCH 1/4] mmc: vub300: fix NULL-deref on disconnect

Quoted 27 lines · Ulf Hansson

Quoted 17 lines · Johan Hovold
Quoted 10 lines
drivers/mmc/host/vub300.c static void vub300_disconnect(struct usb_interface *interface) · line 2369
Quoted 7 lines
drivers/mmc/host/vub300.c +1 -1
@@ -2365,8 +2365,8 @@ static void vub300_disconnect(struct usb_interface *interface)
                        usb_set_intfdata(interface, NULL);
                        /* prevent more I/O from starting */
                        vub300->interface = NULL;
-                       kref_put(&vub300->kref, vub300_delete);
                        mmc_remove_host(mmc);
+                       kref_put(&vub300->kref, vub300_delete);
While this seems like a step in the right direction, I don't see why calling usb_set_intfdata(interface, NULL)
The interface data is only used in the USB bus callbacks and is not needed after disconnect().
Quoted 3 lines
and assigning
vub300->interface = NULL is safe.
Quoted 7 lines
Quoted 2 lines
For example, some of the workqueues might be running a work that uses the vub300->interface, isn't that a problem too?
The driver uses this pointer to indicate that the device has been disconnected. That doesn't mean that the implementation is correct (e.g. the check in vub300_pollwork_thread() should use some locking) but that would be pre-existing issues.

Quoted 4 lines

Right, that was my thinking as well.
Out of curiosity, are you planning on fixing these issues too or is that left for later?

No, sorry, this was just something I stumbled over when addressing USB devres issues tree wide.

Johan

Johan Hovold

author

2026-03-27 10:52 UTC #6 lore raw

PATCH 2/4 mmc: vub300: fix use-after-free on disconnect

In reply to Johan Hovold

The vub300 driver maintains an explicit reference count for the controller and its driver data and the last reference can in theory be dropped after the driver has been unbound.

This specifically means that the controller allocation must not be device managed as that can lead to use-after-free.

Note that the lifetime is currently also incorrectly tied the parent USB device rather than interface, which can lead to memory leaks if the driver is unbound without its device being physically disconnected (e.g. on probe deferral).

Fix both issues by reverting to non-managed allocation of the controller.

Fixes dcfdd698dc52 ("mmc: vub300: Use devm_mmc_alloc_host() helper")

Cc [email protected] # 6.17

Cc Binbin Zhou <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/mmc/host/vub300.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

drivers/mmc/host/vub300.c +11 -6

index f173c7cf4e1a..3c9df27f9fa7 100644
@@ -369,11 +369,14 @@ struct vub300_mmc_host {
 static void vub300_delete(struct kref *kref)
 {				/* kref callback - softirq */
 	struct vub300_mmc_host *vub300 = kref_to_vub300_mmc_host(kref);
+	struct mmc_host *mmc = vub300->mmc;
+
 	usb_free_urb(vub300->command_out_urb);
 	vub300->command_out_urb = NULL;
 	usb_free_urb(vub300->command_res_urb);
 	vub300->command_res_urb = NULL;
 	usb_put_dev(vub300->udev);
+	mmc_free_host(mmc);
 	/*
 	 * and hence also frees vub300
 	 * which is contained at the end of struct mmc

@@ -2112,7 +2115,7 @@ static int vub300_probe(struct usb_interface *interface,
 		goto error1;
 	}
 	/* this also allocates memory for our VUB300 mmc host device */
-	mmc = devm_mmc_alloc_host(&udev->dev, sizeof(*vub300));
+	mmc = mmc_alloc_host(sizeof(*vub300), &udev->dev);
 	if (!mmc) {
 		retval = -ENOMEM;
 		dev_err(&udev->dev, "not enough memory for the mmc_host\n");

@@ -2269,7 +2272,7 @@ static int vub300_probe(struct usb_interface *interface,
 		dev_err(&vub300->udev->dev,
 		    "Could not find two sets of bulk-in/out endpoint pairs\n");
 		retval = -EINVAL;
-		goto error4;
+		goto err_free_host;
 	}
 	retval =
 		usb_control_msg(vub300->udev, usb_rcvctrlpipe(vub300->udev, 0),

@@ -2278,14 +2281,14 @@ static int vub300_probe(struct usb_interface *interface,
 				0x0000, 0x0000, &vub300->hc_info,
 				sizeof(vub300->hc_info), 1000);
 	if (retval < 0)
-		goto error4;
+		goto err_free_host;
 	retval =
 		usb_control_msg(vub300->udev, usb_sndctrlpipe(vub300->udev, 0),
 				SET_ROM_WAIT_STATES,
 				USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 				firmware_rom_wait_states, 0x0000, NULL, 0, 1000);
 	if (retval < 0)
-		goto error4;
+		goto err_free_host;
 	dev_info(&vub300->udev->dev,
 		 "operating_mode = %s %s %d MHz %s %d byte USB packets\n",
 		 (mmc->caps & MMC_CAP_SDIO_IRQ) ? "IRQs" : "POLL",

@@ -2300,7 +2303,7 @@ static int vub300_probe(struct usb_interface *interface,
 				0x0000, 0x0000, &vub300->system_port_status,
 				sizeof(vub300->system_port_status), 1000);
 	if (retval < 0) {
-		goto error4;
+		goto err_free_host;
 	} else if (sizeof(vub300->system_port_status) == retval) {
 		vub300->card_present =
 			(0x0001 & vub300->system_port_status.port_flags) ? 1 : 0;

@@ -2308,7 +2311,7 @@ static int vub300_probe(struct usb_interface *interface,
 			(0x0010 & vub300->system_port_status.port_flags) ? 1 : 0;
 	} else {
 		retval = -EINVAL;
-		goto error4;
+		goto err_free_host;
 	}
 	usb_set_intfdata(interface, vub300);
 	INIT_DELAYED_WORK(&vub300->pollwork, vub300_pollwork_thread);

@@ -2338,6 +2341,8 @@ static int vub300_probe(struct usb_interface *interface,
 	return 0;
 error6:
 	timer_delete_sync(&vub300->inactivity_timer);
+err_free_host:
+	mmc_free_host(mmc);
 	/*
 	 * and hence also frees vub300
 	 * which is contained at the end of struct mmc

Signature · 1 line

2.52.0

Ulf Hansson

2026-03-31 10:25 UTC #7 lore raw

Re: [PATCH 2/4] mmc: vub300: fix use-after-free on disconnect

Quoted 8 lines · Johan Hovold

The vub300 driver maintains an explicit reference count for the controller and its driver data and the last reference can in theory be dropped after the driver has been unbound.
This specifically means that the controller allocation must not be device managed as that can lead to use-after-free.

Quoted 6 lines

Note that the lifetime is currently also incorrectly tied the parent USB device rather than interface, which can lead to memory leaks if the driver is unbound without its device being physically disconnected (e.g. on probe deferral).
Fix both issues by reverting to non-managed allocation of the controller.

Huh, sounds like a real mess, but thanks for the detailed description.

I will defer applying until we finalize the discussion on patch1, but otherwise this looks good to me.

Kind regards
Uffe

Quoted 93 lines

Fixes dcfdd698dc52 ("mmc: vub300: Use devm_mmc_alloc_host() helper")

Cc [email protected] # 6.17

Cc Binbin Zhou <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/mmc/host/vub300.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

drivers/mmc/host/vub300.c +11 -7

index f173c7cf4e1a..3c9df27f9fa7 100644
@@ -369,11 +369,14 @@ struct vub300_mmc_host {
 static void vub300_delete(struct kref *kref)
 {                              /* kref callback - softirq */
        struct vub300_mmc_host *vub300 = kref_to_vub300_mmc_host(kref);
+       struct mmc_host *mmc = vub300->mmc;
+
        usb_free_urb(vub300->command_out_urb);
        vub300->command_out_urb = NULL;
        usb_free_urb(vub300->command_res_urb);
        vub300->command_res_urb = NULL;
        usb_put_dev(vub300->udev);
+       mmc_free_host(mmc);
        /*
         * and hence also frees vub300
         * which is contained at the end of struct mmc

@@ -2112,7 +2115,7 @@ static int vub300_probe(struct usb_interface *interface,
                goto error1;
        }
        /* this also allocates memory for our VUB300 mmc host device */
-       mmc = devm_mmc_alloc_host(&udev->dev, sizeof(*vub300));
+       mmc = mmc_alloc_host(sizeof(*vub300), &udev->dev);
        if (!mmc) {
                retval = -ENOMEM;
                dev_err(&udev->dev, "not enough memory for the mmc_host\n");

@@ -2269,7 +2272,7 @@ static int vub300_probe(struct usb_interface *interface,
                dev_err(&vub300->udev->dev,
                    "Could not find two sets of bulk-in/out endpoint pairs\n");
                retval = -EINVAL;
-               goto error4;
+               goto err_free_host;
        }
        retval =
                usb_control_msg(vub300->udev, usb_rcvctrlpipe(vub300->udev, 0),

@@ -2278,14 +2281,14 @@ static int vub300_probe(struct usb_interface *interface,
                                0x0000, 0x0000, &vub300->hc_info,
                                sizeof(vub300->hc_info), 1000);
        if (retval < 0)
-               goto error4;
+               goto err_free_host;
        retval =
                usb_control_msg(vub300->udev, usb_sndctrlpipe(vub300->udev, 0),
                                SET_ROM_WAIT_STATES,
                                USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
                                firmware_rom_wait_states, 0x0000, NULL, 0, 1000);
        if (retval < 0)
-               goto error4;
+               goto err_free_host;
        dev_info(&vub300->udev->dev,
                 "operating_mode = %s %s %d MHz %s %d byte USB packets\n",
                 (mmc->caps & MMC_CAP_SDIO_IRQ) ? "IRQs" : "POLL",

@@ -2300,7 +2303,7 @@ static int vub300_probe(struct usb_interface *interface,
                                0x0000, 0x0000, &vub300->system_port_status,
                                sizeof(vub300->system_port_status), 1000);
        if (retval < 0) {
-               goto error4;
+               goto err_free_host;
        } else if (sizeof(vub300->system_port_status) == retval) {
                vub300->card_present =
                        (0x0001 & vub300->system_port_status.port_flags) ? 1 : 0;

@@ -2308,7 +2311,7 @@ static int vub300_probe(struct usb_interface *interface,
                        (0x0010 & vub300->system_port_status.port_flags) ? 1 : 0;
        } else {
                retval = -EINVAL;
-               goto error4;
+               goto err_free_host;
        }
        usb_set_intfdata(interface, vub300);
        INIT_DELAYED_WORK(&vub300->pollwork, vub300_pollwork_thread);

@@ -2338,6 +2341,8 @@ static int vub300_probe(struct usb_interface *interface,
        return 0;
 error6:
        timer_delete_sync(&vub300->inactivity_timer);
+err_free_host:
+       mmc_free_host(mmc);
        /*
         * and hence also frees vub300
         * which is contained at the end of struct mmc
--

2.52.0

Johan Hovold

author

2026-03-27 10:52 UTC #8 lore raw

PATCH 3/4 mmc: vub300: rename probe error labels

In reply to Johan Hovold

Error labels should be named after what they do.

Rename the probe error labels.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/mmc/host/vub300.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/mmc/host/vub300.c +10 -8

index 3c9df27f9fa7..3057a69ff8c4 100644
@@ -2107,19 +2107,19 @@ static int vub300_probe(struct usb_interface *interface,
 	command_out_urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!command_out_urb) {
 		retval = -ENOMEM;
-		goto error0;
+		goto err_put_udev;
 	}
 	command_res_urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!command_res_urb) {
 		retval = -ENOMEM;
-		goto error1;
+		goto err_free_out_urb;
 	}
 	/* this also allocates memory for our VUB300 mmc host device */
 	mmc = mmc_alloc_host(sizeof(*vub300), &udev->dev);
 	if (!mmc) {
 		retval = -ENOMEM;
 		dev_err(&udev->dev, "not enough memory for the mmc_host\n");
-		goto error4;
+		goto err_free_res_urb;
 	}
 	/* MMC core transfer sizes tunable parameters */
 	mmc->caps = 0;

@@ -2336,10 +2336,11 @@ static int vub300_probe(struct usb_interface *interface,
 			 interface_to_InterfaceNumber(interface));
 	retval = mmc_add_host(mmc);
 	if (retval)
-		goto error6;
+		goto err_delete_timer;
 
 	return 0;
-error6:
+
+err_delete_timer:
 	timer_delete_sync(&vub300->inactivity_timer);
 err_free_host:
 	mmc_free_host(mmc);

@@ -2347,12 +2348,13 @@ static int vub300_probe(struct usb_interface *interface,
 	 * and hence also frees vub300
 	 * which is contained at the end of struct mmc
 	 */
-error4:
+err_free_res_urb:
 	usb_free_urb(command_res_urb);
-error1:
+err_free_out_urb:
 	usb_free_urb(command_out_urb);
-error0:
+err_put_udev:
 	usb_put_dev(udev);
+
 	return retval;
 }

Signature · 1 line

2.52.0

Johan Hovold

author

2026-03-27 10:52 UTC #9 lore raw

PATCH 4/4 mmc: vub300: clean up module init

In reply to Johan Hovold

Clean up module init by dropping redundant error messages (e.g. allocation and USB driver registration failure will already have been logged) and naming error labels after what they do.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/mmc/host/vub300.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

drivers/mmc/host/vub300.c +16 ~~-17~~

index 3057a69ff8c4..6c3cb2f1c9d3 100644
@@ -2429,37 +2429,36 @@ static int __init vub300_init(void)
 
 	pr_info("VUB300 Driver rom wait states = %02X irqpoll timeout = %04X",
 		firmware_rom_wait_states, 0x0FFFF & firmware_irqpoll_timeout);
+
 	cmndworkqueue = create_singlethread_workqueue("kvub300c");
-	if (!cmndworkqueue) {
-		pr_err("not enough memory for the REQUEST workqueue");
-		result = -ENOMEM;
-		goto out1;
-	}
+	if (!cmndworkqueue)
+		return -ENOMEM;
+
 	pollworkqueue = create_singlethread_workqueue("kvub300p");
 	if (!pollworkqueue) {
-		pr_err("not enough memory for the IRQPOLL workqueue");
 		result = -ENOMEM;
-		goto out2;
+		goto err_destroy_cmdwq;
 	}
+
 	deadworkqueue = create_singlethread_workqueue("kvub300d");
 	if (!deadworkqueue) {
-		pr_err("not enough memory for the EXPIRED workqueue");
 		result = -ENOMEM;
-		goto out3;
+		goto err_destroy_pollwq;
 	}
+
 	result = usb_register(&vub300_driver);
-	if (result) {
-		pr_err("usb_register failed. Error number %d", result);
-		goto out4;
-	}
+	if (result)
+		goto err_destroy_deadwq;
+
 	return 0;
-out4:
+
+err_destroy_deadwq:
 	destroy_workqueue(deadworkqueue);
-out3:
+err_destroy_pollwq:
 	destroy_workqueue(pollworkqueue);
-out2:
+err_destroy_cmdwq:
 	destroy_workqueue(cmndworkqueue);
-out1:
+
 	return result;
 }

Signature · 1 line

2.52.0

Ulf Hansson

2026-03-31 11:15 UTC #10 lore raw

In reply to Johan Hovold

Quoted 3 lines · Johan Hovold

This series fixes lifetime issues that can lead to NULL pointer dereferences and use-after-free.

Patch1 and 2 applied for fixes and patch3 and 4 for next, thanks!

Kind regards
Uffe

Quoted 18 lines

Included are also two related cleanups.
Johan
Johan Hovold (4):
mmc: vub300: fix NULL-deref on disconnect
mmc: vub300: fix use-after-free on disconnect
mmc: vub300: rename probe error labels
mmc: vub300: clean up module init
 drivers/mmc/host/vub300.c | 70 +++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 32 deletions(-)
--
2.52.0