diff mbox series

[RFT,v1] media: zr364xx: Fix memory leak in ->probe()

Message ID 20201230211918.63508-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series [RFT,v1] media: zr364xx: Fix memory leak in ->probe() | expand

Commit Message

Andy Shevchenko Dec. 30, 2020, 9:19 p.m. UTC
When ->probe() fails in some cases it may not free resources.
Replace few separated calls by v4l2_device_put() to clean up
everything.

Reported-by: syzbot+b4d54814b339b5c6bbd4@syzkaller.appspotmail.com
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
I have no hardware and hadn't done any test of this.

 drivers/media/usb/zr364xx/zr364xx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Ezequiel Garcia Dec. 31, 2020, 1:53 a.m. UTC | #1
Hey Andy,

Thank you for the patch.

On Wed, 30 Dec 2020 at 18:22, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>

> When ->probe() fails in some cases it may not free resources.

> Replace few separated calls by v4l2_device_put() to clean up

> everything.

>

> Reported-by: syzbot+b4d54814b339b5c6bbd4@syzkaller.appspotmail.com

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---

> I have no hardware and hadn't done any test of this.

>


For bugs with reproducers such as this one, syzbot will test your
patches really quickly.
Just push the patch somewhere and then reply to syzbot bug report mail with

#syz test: git://repo/address.git commit-hash

You can experiment with syzbot by replying only to syzbot's mail address.

See https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
for more details.

Cheers,
Ezequiel


>  drivers/media/usb/zr364xx/zr364xx.c | 4 +---

>  1 file changed, 1 insertion(+), 3 deletions(-)

>

> diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c

> index 1e1c6b4d1874..5b9e31af57cf 100644

> --- a/drivers/media/usb/zr364xx/zr364xx.c

> +++ b/drivers/media/usb/zr364xx/zr364xx.c

> @@ -1533,9 +1533,7 @@ static int zr364xx_probe(struct usb_interface *intf,

>         return 0;

>

>  fail:

> -       v4l2_ctrl_handler_free(hdl);

> -       v4l2_device_unregister(&cam->v4l2_dev);

> -       kfree(cam);

> +       v4l2_device_put(&cam->v4l2_dev);

>         return err;

>  }

>

> --

> 2.29.2

>
Dan Carpenter Jan. 5, 2021, 2 p.m. UTC | #2
On Wed, Dec 30, 2020 at 11:19:18PM +0200, Andy Shevchenko wrote:
> When ->probe() fails in some cases it may not free resources.

> Replace few separated calls by v4l2_device_put() to clean up

> everything.

> 


The clean up everything style of error handling is always buggy.

For example, in this case, all the early error paths will now crash
instead of leaking.  The __videobuf_free() function will Oops when it
dereferences "q->int_ops->magic".

	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);

The "q->int_ops" pointer is set in videobuf_queue_vmalloc_init().  There
are probably other bugs as well.  It's almost impossible to audit this
style of error handling either for completeness or for crashyness.

regards,
dan carpenter
Andy Shevchenko Jan. 5, 2021, 2:37 p.m. UTC | #3
On Tue, Jan 05, 2021 at 05:00:45PM +0300, Dan Carpenter wrote:
> On Wed, Dec 30, 2020 at 11:19:18PM +0200, Andy Shevchenko wrote:
> > When ->probe() fails in some cases it may not free resources.
> > Replace few separated calls by v4l2_device_put() to clean up
> > everything.
> > 
> 
> The clean up everything style of error handling is always buggy.
> 
> For example, in this case, all the early error paths will now crash
> instead of leaking.  The __videobuf_free() function will Oops when it
> dereferences "q->int_ops->magic".
> 
> 	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
> 
> The "q->int_ops" pointer is set in videobuf_queue_vmalloc_init().  There
> are probably other bugs as well.  It's almost impossible to audit this
> style of error handling either for completeness or for crashyness.

Feel free to submit better fix, thanks!
Dan Carpenter Jan. 5, 2021, 4:04 p.m. UTC | #4
On Tue, Jan 05, 2021 at 04:37:41PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 05, 2021 at 05:00:45PM +0300, Dan Carpenter wrote:
> > On Wed, Dec 30, 2020 at 11:19:18PM +0200, Andy Shevchenko wrote:
> > > When ->probe() fails in some cases it may not free resources.
> > > Replace few separated calls by v4l2_device_put() to clean up
> > > everything.
> > > 
> > 
> > The clean up everything style of error handling is always buggy.
> > 
> > For example, in this case, all the early error paths will now crash
> > instead of leaking.  The __videobuf_free() function will Oops when it
> > dereferences "q->int_ops->magic".
> > 
> > 	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
> > 
> > The "q->int_ops" pointer is set in videobuf_queue_vmalloc_init().  There
> > are probably other bugs as well.  It's almost impossible to audit this
> > style of error handling either for completeness or for crashyness.
> 
> Feel free to submit better fix, thanks!

Sure.  I'm too tired to think straight today.

I see now that syzbot actually discovered the Oops in __videobuf_free()
as well...  I'm sort of surprised that the original code never called
zr364xx_release().  We might have another reference leak somewhere...

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c
index 1e1c6b4d1874..5b9e31af57cf 100644
--- a/drivers/media/usb/zr364xx/zr364xx.c
+++ b/drivers/media/usb/zr364xx/zr364xx.c
@@ -1533,9 +1533,7 @@  static int zr364xx_probe(struct usb_interface *intf,
 	return 0;
 
 fail:
-	v4l2_ctrl_handler_free(hdl);
-	v4l2_device_unregister(&cam->v4l2_dev);
-	kfree(cam);
+	v4l2_device_put(&cam->v4l2_dev);
 	return err;
 }