| 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 |
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 >
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
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!
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 --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; }
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(-)