* [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe()
@ 2020-12-30 21:19 Andy Shevchenko
2020-12-31 1:53 ` Ezequiel Garcia
2021-01-05 14:00 ` Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-12-30 21:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Media Mailing List, royale, USB,
syzkaller-bugs
Cc: Andy Shevchenko, syzbot+b4d54814b339b5c6bbd4
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe()
2020-12-30 21:19 [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe() Andy Shevchenko
@ 2020-12-31 1:53 ` Ezequiel Garcia
2020-12-31 1:53 ` syzbot
2021-01-05 14:00 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Ezequiel Garcia @ 2020-12-31 1:53 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mauro Carvalho Chehab, Linux Media Mailing List, royale, USB,
syzkaller-bugs, syzbot
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe()
2020-12-31 1:53 ` Ezequiel Garcia
@ 2020-12-31 1:53 ` syzbot
0 siblings, 0 replies; 6+ messages in thread
From: syzbot @ 2020-12-31 1:53 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: andriy.shevchenko, ezequiel, linux-media, linux-usb, mchehab,
royale, syzkaller-bugs
> 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
"git://repo/address.git" does not look like a valid git repo address.
>
> 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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe()
2020-12-30 21:19 [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe() Andy Shevchenko
2020-12-31 1:53 ` Ezequiel Garcia
@ 2021-01-05 14:00 ` Dan Carpenter
2021-01-05 14:37 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-01-05 14:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mauro Carvalho Chehab, Linux Media Mailing List, royale, USB,
syzkaller-bugs, syzbot+b4d54814b339b5c6bbd4
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe()
2021-01-05 14:00 ` Dan Carpenter
@ 2021-01-05 14:37 ` Andy Shevchenko
2021-01-05 16:04 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2021-01-05 14:37 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Linux Media Mailing List, royale, USB,
syzkaller-bugs, syzbot+b4d54814b339b5c6bbd4
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!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe()
2021-01-05 14:37 ` Andy Shevchenko
@ 2021-01-05 16:04 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-01-05 16:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mauro Carvalho Chehab, Linux Media Mailing List, royale, USB,
syzkaller-bugs, syzbot+b4d54814b339b5c6bbd4
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-05 16:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-30 21:19 [RFT][PATCH v1] media: zr364xx: Fix memory leak in ->probe() Andy Shevchenko
2020-12-31 1:53 ` Ezequiel Garcia
2020-12-31 1:53 ` syzbot
2021-01-05 14:00 ` Dan Carpenter
2021-01-05 14:37 ` Andy Shevchenko
2021-01-05 16:04 ` Dan Carpenter
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).