[PATCH 0/2] media: dvb-usb: Fix UAF and memory leaks

[PATCH 0/2] media: dvb-usb: Fix UAF and memory leaks

[Takashi Iwai]

Hi,

here is a patch set to address the use-after-free at disconnecting
a USB DVB device that was recently reported on openSUSE Bugzilla.
The bug itself seems to be a long-standing one, and I spotted
another memory leak there, which is covered in the first patch.


Takashi

===

Takashi Iwai (2):
  media: dvb-usb: Fix memory leak at error in dvb_usb_device_init()
  media: dvb-usb: Fix use-after-free access

 drivers/media/usb/dvb-usb/dvb-usb-init.c | 41 +++++++++++++++---------
 1 file changed, 25 insertions(+), 16 deletions(-)

-- 
2.26.2

[PATCH 1/2] media: dvb-usb: Fix memory leak at error in dvb_usb_device_init()

[Takashi Iwai]

dvb_usb_device_init() allocates a dvb_usb_device object, but it
doesn't release it even when returning an error.  The callers don't
seem caring it as well, hence those memories are leaked.

This patch assures releasing the memory at the error path in
dvb_usb_device_init().  Also it makes sure that USB intfdata is reset
and don't return the bogus pointer to the caller at the error path,
too.

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/media/usb/dvb-usb/dvb-usb-init.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
index c1a7634e27b4..5befec87f26a 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
@@ -281,15 +281,21 @@ int dvb_usb_device_init(struct usb_interface *intf,
 
 	usb_set_intfdata(intf, d);
 
-	if (du != NULL)
+	ret = dvb_usb_init(d, adapter_nums);
+	if (ret) {
+		info("%s error while loading driver (%d)", desc->name, ret);
+		goto error;
+	}
+
+	if (du)
 		*du = d;
 
-	ret = dvb_usb_init(d, adapter_nums);
+	info("%s successfully initialized and connected.", desc->name);
+	return 0;
 
-	if (ret == 0)
-		info("%s successfully initialized and connected.", desc->name);
-	else
-		info("%s error while loading driver (%d)", desc->name, ret);
+ error:
+	usb_set_intfdata(intf, NULL);
+	kfree(d);
 	return ret;
 }
 EXPORT_SYMBOL(dvb_usb_device_init);
-- 
2.26.2

Re: [PATCH 1/2] media: dvb-usb: Fix memory leak at error in dvb_usb_device_init()

[Robert Foss]

Hey Takashi,

Thanks for the patch. It looks good to me, feel free to add my r-b.
Reviewed-by: Robert Foss <robert.foss@linaro.org>


On Wed, 20 Jan 2021 at 12:51, Takashi Iwai <tiwai@suse.de> wrote:
>
> dvb_usb_device_init() allocates a dvb_usb_device object, but it
> doesn't release it even when returning an error.  The callers don't
> seem caring it as well, hence those memories are leaked.
>
> This patch assures releasing the memory at the error path in
> dvb_usb_device_init().  Also it makes sure that USB intfdata is reset
> and don't return the bogus pointer to the caller at the error path,
> too.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/media/usb/dvb-usb/dvb-usb-init.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> index c1a7634e27b4..5befec87f26a 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> @@ -281,15 +281,21 @@ int dvb_usb_device_init(struct usb_interface *intf,
>
>         usb_set_intfdata(intf, d);
>
> -       if (du != NULL)
> +       ret = dvb_usb_init(d, adapter_nums);
> +       if (ret) {
> +               info("%s error while loading driver (%d)", desc->name, ret);
> +               goto error;
> +       }
> +
> +       if (du)
>                 *du = d;
>
> -       ret = dvb_usb_init(d, adapter_nums);
> +       info("%s successfully initialized and connected.", desc->name);
> +       return 0;
>
> -       if (ret == 0)
> -               info("%s successfully initialized and connected.", desc->name);
> -       else
> -               info("%s error while loading driver (%d)", desc->name, ret);
> + error:
> +       usb_set_intfdata(intf, NULL);
> +       kfree(d);
>         return ret;
>  }
>  EXPORT_SYMBOL(dvb_usb_device_init);
> --
> 2.26.2
>

Re: [PATCH 1/2] media: dvb-usb: Fix memory leak at error in dvb_usb_device_init()

[Sean Young]

On Wed, Jan 20, 2021 at 11:20:56AM +0100, Takashi Iwai wrote:
> dvb_usb_device_init() allocates a dvb_usb_device object, but it
> doesn't release it even when returning an error.  The callers don't
> seem caring it as well, hence those memories are leaked.
> 
> This patch assures releasing the memory at the error path in
> dvb_usb_device_init().  Also it makes sure that USB intfdata is reset
> and don't return the bogus pointer to the caller at the error path,
> too.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/media/usb/dvb-usb/dvb-usb-init.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> index c1a7634e27b4..5befec87f26a 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> @@ -281,15 +281,21 @@ int dvb_usb_device_init(struct usb_interface *intf,
>  
>  	usb_set_intfdata(intf, d);
>  
> -	if (du != NULL)
> +	ret = dvb_usb_init(d, adapter_nums);

dvb_usb_init() has different errors paths. 

1. It can return -ENOMEM if it cannot kzalloc(). No other side affects.
2. It can return an error if dvb_usb_i2c_init() or dvb_usb_adapter_init()
   fails. In this case, dvb_usb_exit() is called, which frees 
   struct dvb_usb_device*

In the last case we now have a double free.


Sean

> +	if (ret) {
> +		info("%s error while loading driver (%d)", desc->name, ret);
> +		goto error;
> +	}
> +
> +	if (du)
>  		*du = d;
>  
> -	ret = dvb_usb_init(d, adapter_nums);
> +	info("%s successfully initialized and connected.", desc->name);
> +	return 0;
>  
> -	if (ret == 0)
> -		info("%s successfully initialized and connected.", desc->name);
> -	else
> -		info("%s error while loading driver (%d)", desc->name, ret);
> + error:
> +	usb_set_intfdata(intf, NULL);
> +	kfree(d);
>  	return ret;
>  }
>  EXPORT_SYMBOL(dvb_usb_device_init);

> -- 
> 2.26.2

Re: [PATCH 1/2] media: dvb-usb: Fix memory leak at error in dvb_usb_device_init()

[Takashi Iwai]

On Sun, 31 Jan 2021 15:53:20 +0100,
Sean Young wrote:
> 
> On Wed, Jan 20, 2021 at 11:20:56AM +0100, Takashi Iwai wrote:
> > dvb_usb_device_init() allocates a dvb_usb_device object, but it
> > doesn't release it even when returning an error.  The callers don't
> > seem caring it as well, hence those memories are leaked.
> > 
> > This patch assures releasing the memory at the error path in
> > dvb_usb_device_init().  Also it makes sure that USB intfdata is reset
> > and don't return the bogus pointer to the caller at the error path,
> > too.
> > 
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/media/usb/dvb-usb/dvb-usb-init.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > index c1a7634e27b4..5befec87f26a 100644
> > --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > @@ -281,15 +281,21 @@ int dvb_usb_device_init(struct usb_interface *intf,
> >  
> >  	usb_set_intfdata(intf, d);
> >  
> > -	if (du != NULL)
> > +	ret = dvb_usb_init(d, adapter_nums);
> 
> dvb_usb_init() has different errors paths. 
> 
> 1. It can return -ENOMEM if it cannot kzalloc(). No other side affects.
> 2. It can return an error if dvb_usb_i2c_init() or dvb_usb_adapter_init()
>    fails. In this case, dvb_usb_exit() is called, which frees 
>    struct dvb_usb_device*
> 
> In the last case we now have a double free.

A good catch, indeed the function has inconsistent behavior.
I'll update the patch and resubmit to address it.


thanks,

Takashi

[PATCH 2/2] media: dvb-usb: Fix use-after-free access

[Takashi Iwai]

dvb_usb_device_init() copies the properties to the own data, so that
the callers can release the original properties later (as done in the
commit 299c7007e936 "media: dw2102: Fix memleak on sequence of
probes").  However, it also stores dev->desc pointer that is a
reference to the original properties data.  Since dev->desc is
referred later, it may result in use-after-free, in the worst case,
leading to a kernel Oops as reported.

This patch addresses the problem by allocating and copying the
properties at first, then get the desc from the copied properties.

Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1181104
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/media/usb/dvb-usb/dvb-usb-init.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
index 5befec87f26a..07ff9b4d2f34 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
@@ -255,27 +255,30 @@ int dvb_usb_device_init(struct usb_interface *intf,
 	if (du != NULL)
 		*du = NULL;
 
-	if ((desc = dvb_usb_find_device(udev, props, &cold)) == NULL) {
+	d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
+	if (!d) {
+		err("no memory for 'struct dvb_usb_device'");
+		return -ENOMEM;
+	}
+
+	memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
+
+	desc = dvb_usb_find_device(udev, &d->props, &cold);
+	if (!desc) {
 		deb_err("something went very wrong, device was not found in current device list - let's see what comes next.\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto error;
 	}
 
 	if (cold) {
 		info("found a '%s' in cold state, will try to load a firmware", desc->name);
 		ret = dvb_usb_download_firmware(udev, props);
 		if (!props->no_reconnect || ret != 0)
-			return ret;
+			goto error;
 	}
 
 	info("found a '%s' in warm state.", desc->name);
-	d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
-	if (d == NULL) {
-		err("no memory for 'struct dvb_usb_device'");
-		return -ENOMEM;
-	}
-
 	d->udev = udev;
-	memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
 	d->desc = desc;
 	d->owner = owner;
 
-- 
2.26.2

Re: [PATCH 2/2] media: dvb-usb: Fix use-after-free access

[Robert Foss]

Hey Takashi,

This patch is generating a checkpatch warning, but I think it is
spurious and can be ignored.

Other than that, this looks good to me.
Reviewed-by: Robert Foss <robert.foss@linaro.org>

On Wed, 20 Jan 2021 at 12:51, Takashi Iwai <tiwai@suse.de> wrote:
>
> dvb_usb_device_init() copies the properties to the own data, so that
> the callers can release the original properties later (as done in the
> commit 299c7007e936 "media: dw2102: Fix memleak on sequence of
> probes").  However, it also stores dev->desc pointer that is a
> reference to the original properties data.  Since dev->desc is
> referred later, it may result in use-after-free, in the worst case,
> leading to a kernel Oops as reported.
>
> This patch addresses the problem by allocating and copying the
> properties at first, then get the desc from the copied properties.
>
> Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
> BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1181104
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/media/usb/dvb-usb/dvb-usb-init.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> index 5befec87f26a..07ff9b4d2f34 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> @@ -255,27 +255,30 @@ int dvb_usb_device_init(struct usb_interface *intf,
>         if (du != NULL)
>                 *du = NULL;
>
> -       if ((desc = dvb_usb_find_device(udev, props, &cold)) == NULL) {
> +       d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
> +       if (!d) {
> +               err("no memory for 'struct dvb_usb_device'");
> +               return -ENOMEM;
> +       }
> +
> +       memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
> +
> +       desc = dvb_usb_find_device(udev, &d->props, &cold);
> +       if (!desc) {
>                 deb_err("something went very wrong, device was not found in current device list - let's see what comes next.\n");
> -               return -ENODEV;
> +               ret = -ENODEV;
> +               goto error;
>         }
>
>         if (cold) {
>                 info("found a '%s' in cold state, will try to load a firmware", desc->name);
>                 ret = dvb_usb_download_firmware(udev, props);
>                 if (!props->no_reconnect || ret != 0)
> -                       return ret;
> +                       goto error;
>         }
>
>         info("found a '%s' in warm state.", desc->name);
> -       d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
> -       if (d == NULL) {
> -               err("no memory for 'struct dvb_usb_device'");
> -               return -ENOMEM;
> -       }
> -
>         d->udev = udev;
> -       memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
>         d->desc = desc;
>         d->owner = owner;
>
> --
> 2.26.2
>

Re: [PATCH 2/2] media: dvb-usb: Fix use-after-free access

[Sean Young]

Hi Takashi,

On Fri, Jan 22, 2021 at 04:47:44PM +0100, Robert Foss wrote:
> Hey Takashi,
> 
> This patch is generating a checkpatch warning, but I think it is
> spurious and can be ignored.

The checkpatch warning isn't superious and should really be corrected.

> 
> Other than that, this looks good to me.
> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> 
> On Wed, 20 Jan 2021 at 12:51, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > dvb_usb_device_init() copies the properties to the own data, so that
> > the callers can release the original properties later (as done in the
> > commit 299c7007e936 "media: dw2102: Fix memleak on sequence of
> > probes").  However, it also stores dev->desc pointer that is a
> > reference to the original properties data.  Since dev->desc is
> > referred later, it may result in use-after-free, in the worst case,
> > leading to a kernel Oops as reported.
> >
> > This patch addresses the problem by allocating and copying the
> > properties at first, then get the desc from the copied properties.
> >
> > Reported-and-tested-by: Stefan Seyfried <seife+kernel@b1-systems.com>
> > BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1181104
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>

Thank you for your patch. Unfortunately, it depends on the first patch
in the series, which I think has problems (see email about this).

Thanks

Sean


> > ---
> >  drivers/media/usb/dvb-usb/dvb-usb-init.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > index 5befec87f26a..07ff9b4d2f34 100644
> > --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > @@ -255,27 +255,30 @@ int dvb_usb_device_init(struct usb_interface *intf,
> >         if (du != NULL)
> >                 *du = NULL;
> >
> > -       if ((desc = dvb_usb_find_device(udev, props, &cold)) == NULL) {
> > +       d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
> > +       if (!d) {
> > +               err("no memory for 'struct dvb_usb_device'");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
> > +
> > +       desc = dvb_usb_find_device(udev, &d->props, &cold);
> > +       if (!desc) {
> >                 deb_err("something went very wrong, device was not found in current device list - let's see what comes next.\n");
> > -               return -ENODEV;
> > +               ret = -ENODEV;
> > +               goto error;
> >         }
> >
> >         if (cold) {
> >                 info("found a '%s' in cold state, will try to load a firmware", desc->name);
> >                 ret = dvb_usb_download_firmware(udev, props);
> >                 if (!props->no_reconnect || ret != 0)
> > -                       return ret;
> > +                       goto error;
> >         }
> >
> >         info("found a '%s' in warm state.", desc->name);
> > -       d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
> > -       if (d == NULL) {
> > -               err("no memory for 'struct dvb_usb_device'");
> > -               return -ENOMEM;
> > -       }
> > -
> >         d->udev = udev;
> > -       memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
> >         d->desc = desc;
> >         d->owner = owner;
> >
> > --
> > 2.26.2
> >

Re: [PATCH 2/2] media: dvb-usb: Fix use-after-free access

[Takashi Iwai]

On Sun, 31 Jan 2021 16:04:56 +0100,
Sean Young wrote:
> 
> Hi Takashi,
> 
> On Fri, Jan 22, 2021 at 04:47:44PM +0100, Robert Foss wrote:
> > Hey Takashi,
> > 
> > This patch is generating a checkpatch warning, but I think it is
> > spurious and can be ignored.
> 
> The checkpatch warning isn't superious and should really be corrected.

It's case-by-case, checkpatch is no bible by itself.  In this
particular case, it was rather a false-positive of checkpatch: the
commit reference including a line-break.

This issue has been always annoying and I wish this will be dropped
from checkpatch in near future...


thanks,

Takashi