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: [email protected]
Signed-off-by: Pavel Skripkin <[email protected]>
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
Em Sat, 27 Feb 2021 02:37:31 +0300
Pavel Skripkin <[email protected]> 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: [email protected]
> Signed-off-by: Pavel Skripkin <[email protected]>
> 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 <[email protected]> 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: [email protected]
> > Signed-off-by: Pavel Skripkin <[email protected]>
> > 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