| Message ID | 20210226233731.614553-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers/media/usb/gspca/stv06xx: fix memory leak | expand |
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
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,
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)
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(-)