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
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: <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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
Hey Takashi,
Thanks for the patch. It looks good to me, feel free to add my r-b.
Reviewed-by: Robert Foss <[email protected]>
On Wed, 20 Jan 2021 at 12:51, Takashi Iwai <[email protected]> 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: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> 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
>
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: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> 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
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: <[email protected]>
> > Signed-off-by: Takashi Iwai <[email protected]>
> > ---
> > 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