* [PATCH] drivers/media/usb/gspca/stv06xx: fix memory leak
@ 2021-02-26 23:37 Pavel Skripkin
2021-03-23 16:13 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 3+ messages in thread
From: Pavel Skripkin @ 2021-02-26 23:37 UTC (permalink / raw)
To: hverkuil, mchehab, linux-media
Cc: linux-kernel, Pavel Skripkin, syzbot+e7f4c64a4248a0340c37
Syzbot reported memory leak in hdcs_probe_1x00()[1].
hdcs_probe_1x00() allocates memory for struct hdcs, but if hdcs_init() fails in gspca_dev_probe2()
this memory becomes leaked.
int gspca_dev_probe2(struct usb_interface *intf,
const struct usb_device_id *id,
const struct sd_desc *sd_desc,
int dev_size,
struct module *module)
{
...
ret = sd_desc->config(gspca_dev, id);
if (ret < 0)
goto out;
ret = sd_desc->init(gspca_dev);
if (ret < 0)
goto out;
...
out:
if (gspca_dev->input_dev)
input_unregister_device(gspca_dev->input_dev);
v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
v4l2_device_unregister(&gspca_dev->v4l2_dev);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
return ret;
}
Reported-by: syzbot+e7f4c64a4248a0340c37@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
Change-Id: Ia198671177ee346de61780813025110c7c491d7a
---
drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c
index 5a47dcbf1c8e..24df13b33a02 100644
--- a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c
+++ b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c
@@ -485,7 +485,7 @@ static int hdcs_init(struct sd *sd)
stv_bridge_init[i][1]);
}
if (err < 0)
- return err;
+ goto error;
/* sensor soft reset */
hdcs_reset(sd);
@@ -496,12 +496,12 @@ static int hdcs_init(struct sd *sd)
stv_sensor_init[i][1]);
}
if (err < 0)
- return err;
+ goto error;
/* Enable continuous frame capture, bit 2: stop when frame complete */
err = stv06xx_write_sensor(sd, HDCS_REG_CONFIG(sd), BIT(3));
if (err < 0)
- return err;
+ goto error;
/* Set PGA sample duration
(was 0x7E for the STV602, but caused slow framerate with HDCS-1020) */
@@ -512,9 +512,13 @@ static int hdcs_init(struct sd *sd)
err = stv06xx_write_sensor(sd, HDCS_TCTRL,
(HDCS_ADC_START_SIG_DUR << 5) | hdcs->psmp);
if (err < 0)
- return err;
+ goto error;
return hdcs_set_size(sd, hdcs->array.width, hdcs->array.height);
+
+error:
+ kfree(hdcs);
+ return err;
}
static int hdcs_dump(struct sd *sd)
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] drivers/media/usb/gspca/stv06xx: fix memory leak 2021-02-26 23:37 [PATCH] drivers/media/usb/gspca/stv06xx: fix memory leak Pavel Skripkin @ 2021-03-23 16:13 ` Mauro Carvalho Chehab 2021-03-26 20:08 ` Pavel Skripkin 0 siblings, 1 reply; 3+ messages in thread From: Mauro Carvalho Chehab @ 2021-03-23 16:13 UTC (permalink / raw) To: Pavel Skripkin Cc: hverkuil, linux-media, linux-kernel, syzbot+e7f4c64a4248a0340c37 Em Sat, 27 Feb 2021 02:37:31 +0300 Pavel Skripkin <paskripkin@gmail.com> escreveu: > Syzbot reported memory leak in hdcs_probe_1x00()[1]. > hdcs_probe_1x00() allocates memory for struct hdcs, but if hdcs_init() fails in gspca_dev_probe2() > this memory becomes leaked. > > int gspca_dev_probe2(struct usb_interface *intf, > const struct usb_device_id *id, > const struct sd_desc *sd_desc, > int dev_size, > struct module *module) > { > ... > > ret = sd_desc->config(gspca_dev, id); > if (ret < 0) > goto out; > ret = sd_desc->init(gspca_dev); > if (ret < 0) > goto out; > ... > out: > if (gspca_dev->input_dev) > input_unregister_device(gspca_dev->input_dev); > v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler); > v4l2_device_unregister(&gspca_dev->v4l2_dev); > kfree(gspca_dev->usb_buf); > kfree(gspca_dev); > return ret; > } > > Reported-by: syzbot+e7f4c64a4248a0340c37@syzkaller.appspotmail.com > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > Change-Id: Ia198671177ee346de61780813025110c7c491d7a > --- > drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c > index 5a47dcbf1c8e..24df13b33a02 100644 > --- a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c > +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c > @@ -485,7 +485,7 @@ static int hdcs_init(struct sd *sd) > stv_bridge_init[i][1]); > } > if (err < 0) > - return err; > + goto error; > > /* sensor soft reset */ > hdcs_reset(sd); > @@ -496,12 +496,12 @@ static int hdcs_init(struct sd *sd) > stv_sensor_init[i][1]); > } > if (err < 0) > - return err; > + goto error; > > /* Enable continuous frame capture, bit 2: stop when frame complete */ > err = stv06xx_write_sensor(sd, HDCS_REG_CONFIG(sd), BIT(3)); > if (err < 0) > - return err; > + goto error; > > /* Set PGA sample duration > (was 0x7E for the STV602, but caused slow framerate with HDCS-1020) */ > @@ -512,9 +512,13 @@ static int hdcs_init(struct sd *sd) > err = stv06xx_write_sensor(sd, HDCS_TCTRL, > (HDCS_ADC_START_SIG_DUR << 5) | hdcs->psmp); > if (err < 0) > - return err; > + goto error; > > return hdcs_set_size(sd, hdcs->array.width, hdcs->array.height); > + > +error: > + kfree(hdcs); > + return err; > } This doesn't seem the right fix here, as it is not the _init function that allocates it. Also, when the device is disconnected, a memory leak will happen. I suspect that the right fix would be to move this: hdcs = kmalloc(sizeof(struct hdcs), GFP_KERNEL); if (!hdcs) return -ENOMEM; To the main driver (stv06xx.c) - probably replacing it by kzalloc(), and then handle the free code both both sd_probe() and sd_disconnect(). Thanks, Mauro ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drivers/media/usb/gspca/stv06xx: fix memory leak 2021-03-23 16:13 ` Mauro Carvalho Chehab @ 2021-03-26 20:08 ` Pavel Skripkin 0 siblings, 0 replies; 3+ messages in thread From: Pavel Skripkin @ 2021-03-26 20:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: hverkuil, linux-media, linux-kernel, syzbot+e7f4c64a4248a0340c37 Hi! Thanks for the review. On Tue, 2021-03-23 at 17:13 +0100, Mauro Carvalho Chehab wrote: > Em Sat, 27 Feb 2021 02:37:31 +0300 > Pavel Skripkin <paskripkin@gmail.com> escreveu: > > > Syzbot reported memory leak in hdcs_probe_1x00()[1]. > > hdcs_probe_1x00() allocates memory for struct hdcs, but if > > hdcs_init() fails in gspca_dev_probe2() > > this memory becomes leaked. > > > > int gspca_dev_probe2(struct usb_interface *intf, > > const struct usb_device_id *id, > > const struct sd_desc *sd_desc, > > int dev_size, > > struct module *module) > > { > > ... > > > > ret = sd_desc->config(gspca_dev, id); > > if (ret < 0) > > goto out; > > ret = sd_desc->init(gspca_dev); > > if (ret < 0) > > goto out; > > ... > > out: > > if (gspca_dev->input_dev) > > input_unregister_device(gspca_dev->input_dev); > > v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler); > > v4l2_device_unregister(&gspca_dev->v4l2_dev); > > kfree(gspca_dev->usb_buf); > > kfree(gspca_dev); > > return ret; > > } > > > > Reported-by: syzbot+e7f4c64a4248a0340c37@syzkaller.appspotmail.com > > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > > Change-Id: Ia198671177ee346de61780813025110c7c491d7a > > --- > > drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c > > b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c > > index 5a47dcbf1c8e..24df13b33a02 100644 > > --- a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c > > +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c > > @@ -485,7 +485,7 @@ static int hdcs_init(struct sd *sd) > > stv_bridge_init[i][1]); > > } > > if (err < 0) > > - return err; > > + goto error; > > > > /* sensor soft reset */ > > hdcs_reset(sd); > > @@ -496,12 +496,12 @@ static int hdcs_init(struct sd *sd) > > stv_sensor_init[i][1]); > > } > > if (err < 0) > > - return err; > > + goto error; > > > > /* Enable continuous frame capture, bit 2: stop when frame > > complete */ > > err = stv06xx_write_sensor(sd, HDCS_REG_CONFIG(sd), BIT(3)); > > if (err < 0) > > - return err; > > + goto error; > > > > /* Set PGA sample duration > > (was 0x7E for the STV602, but caused slow framerate with > > HDCS-1020) */ > > @@ -512,9 +512,13 @@ static int hdcs_init(struct sd *sd) > > err = stv06xx_write_sensor(sd, HDCS_TCTRL, > > (HDCS_ADC_START_SIG_DUR << 5) | hdcs- > > >psmp); > > if (err < 0) > > - return err; > > + goto error; > > > > return hdcs_set_size(sd, hdcs->array.width, hdcs- > > >array.height); > > + > > +error: > > + kfree(hdcs); > > + return err; > > } > > This doesn't seem the right fix here, as it is not the _init function > that allocates it. Also, when the device is disconnected, a memory > leak > will happen. It won't. static int hdcs_probe_1x00(struct sd *sd) { .... sd->sensor_priv = hdcs; .... } static void sd_disconnect(struct usb_interface *intf) { void *priv = sd->sensor_priv; .... kfree(priv); } Is it correct? > > I suspect that the right fix would be to move this: > > hdcs = kmalloc(sizeof(struct hdcs), GFP_KERNEL); > if (!hdcs) > return -ENOMEM; > > To the main driver (stv06xx.c) - probably replacing it by kzalloc(), > I don't really understand why, because this allocation refers only to stv06xx_hdcs, and other stv06xx sensors don't use it. If hdcs_init() fails, we won't be able to access this pointer, because in gspca_dev_probe2() only this code will be executed on error condition: v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler); v4l2_device_unregister(&gspca_dev->v4l2_dev); kfree(gspca_dev->usb_buf); kfree(gspca_dev); Maybe, I don't properly understand it, can You explain, please? > and then handle the free code both both sd_probe() and sd_disconnect(). > > > Thanks, -- With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-26 20:09 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-26 23:37 [PATCH] drivers/media/usb/gspca/stv06xx: fix memory leak Pavel Skripkin 2021-03-23 16:13 ` Mauro Carvalho Chehab 2021-03-26 20:08 ` Pavel Skripkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).