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

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

Posted by Johan Hovold 6 days, 6 hours ago

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

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

Posted by Ulf Hansson 2 days, 6 hours ago

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 1/4] mmc: vub300: fix NULL-deref on disconnect

Posted by Johan Hovold 2 days, 6 hours ago

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

Posted by Ulf Hansson 2 days, 5 hours ago

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

Posted by Johan Hovold 2 days, 5 hours ago

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