| Message ID | 20210120102057.21143-2-tiwai@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | media: dvb-usb: Fix UAF and memory leaks | expand |
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 >
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
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
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);
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(-)