linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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; 4+ 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] 4+ 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; 4+ 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] 4+ 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; 4+ 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] 4+ messages in thread
* [PATCH] drivers/media/usb/gspca/stv06xx: fix memory leak
@ 2021-02-26 23:37 Pavel Skripkin
  0 siblings, 0 replies; 4+ messages in thread
From: Pavel Skripkin @ 2021-02-26 23:37 UTC (permalink / raw)
  To: blacktea322; +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] 4+ messages in thread
end of thread, other threads:[~2021-03-26 20:09 UTC | newest]

Thread overview: 4+ 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
  -- strict thread matches above, loose matches on Subject: below --
2021-02-26 23:37 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).