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

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

[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: stable@vger.kernel.org	# 3.0
Cc: Tony Olech <tony.olech@elandigitalsystems.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mmc/host/vub300.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index ff49d0770506..f173c7cf4e1a 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -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;
-- 
2.52.0

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

[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: stable@vger.kernel.org	# 6.17
Cc: Binbin Zhou <zhoubinbin@loongson.cn>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mmc/host/vub300.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index f173c7cf4e1a..3c9df27f9fa7 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -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

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

[Ulf Hansson]

On Fri, 27 Mar 2026 at 11:52, Johan Hovold <johan@kernel.org> wrote:
>
> 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: stable@vger.kernel.org      # 3.0
> Cc: Tony Olech <tony.olech@elandigitalsystems.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/mmc/host/vub300.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
> index ff49d0770506..f173c7cf4e1a 100644
> --- a/drivers/mmc/host/vub300.c
> +++ b/drivers/mmc/host/vub300.c
> @@ -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) 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?

>                         pr_info("USB vub300 remote SDIO host controller[%d]"
>                                 " now disconnected", ifnum);
>                         return;
> --
> 2.52.0
>

Kind regards
Uffe

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

[Ulf Hansson]

On Fri, 27 Mar 2026 at 11:52, Johan Hovold <johan@kernel.org> wrote:
>
> 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.

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

>
> Fixes: dcfdd698dc52 ("mmc: vub300: Use devm_mmc_alloc_host() helper")
> Cc: stable@vger.kernel.org      # 6.17
> Cc: Binbin Zhou <zhoubinbin@loongson.cn>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/mmc/host/vub300.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
> index f173c7cf4e1a..3c9df27f9fa7 100644
> --- a/drivers/mmc/host/vub300.c
> +++ b/drivers/mmc/host/vub300.c
> @@ -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
>

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

[Johan Hovold]

On Tue, Mar 31, 2026 at 12:13:41PM +0200, Ulf Hansson wrote:
> On Fri, 27 Mar 2026 at 11:52, Johan Hovold <johan@kernel.org> wrote:
> >
> > 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: stable@vger.kernel.org      # 3.0
> > Cc: Tony Olech <tony.olech@elandigitalsystems.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/mmc/host/vub300.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
> > index ff49d0770506..f173c7cf4e1a 100644
> > --- a/drivers/mmc/host/vub300.c
> > +++ b/drivers/mmc/host/vub300.c
> > @@ -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().

> 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

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

[Ulf Hansson]

On Tue, 31 Mar 2026 at 12:32, Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Mar 31, 2026 at 12:13:41PM +0200, Ulf Hansson wrote:
> > On Fri, 27 Mar 2026 at 11:52, Johan Hovold <johan@kernel.org> wrote:
> > >
> > > 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: stable@vger.kernel.org      # 3.0
> > > Cc: Tony Olech <tony.olech@elandigitalsystems.com>
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > >  drivers/mmc/host/vub300.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
> > > index ff49d0770506..f173c7cf4e1a 100644
> > > --- a/drivers/mmc/host/vub300.c
> > > +++ b/drivers/mmc/host/vub300.c
> > > @@ -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().
>
> > 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.

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

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

[Johan Hovold]

On Tue, Mar 31, 2026 at 01:03:39PM +0200, Ulf Hansson wrote:
> On Tue, 31 Mar 2026 at 12:32, Johan Hovold <johan@kernel.org> wrote:

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