media: zr364xx: Fix memory leak in ->probe()

PATCH In review Andy Shevchenko 4 participants 6 messages Dec 30, 2020 – Jan 5, 2021

Subsystems (2)

Media (V4L/DVB) USB

lore

Share & tools

lore

Andy Shevchenko

author

2020-12-30 21:21 UTC #0 lore raw

PATCH 1/1 media: zr364xx: Fix memory leak in ->probe()

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 [email protected]

Signed-off-by Andy Shevchenko <[email protected]>

---
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(-)

drivers/media/usb/zr364xx/zr364xx.c +1 -3

index 1e1c6b4d1874..5b9e31af57cf 100644
@@ -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;
 }

Signature · 1 line

2.29.2

syzbot

2020-12-31 01:54 UTC #2 lore raw

"git://repo/address.git" does not look like a valid git repo address.

Quoted 17 lines

Hey Andy,
Thank you for the patch.
Quoted 10 lines · Andy Shevchenko
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 [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
I have no hardware and hadn't done any test of this.

Quoted 5 lines

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.

Quoted 31 lines

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
Quoted 21 lines
 drivers/media/usb/zr364xx/zr364xx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/media/usb/zr364xx/zr364xx.c +1 -4
index 1e1c6b4d1874..5b9e31af57cf 100644
@@ -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

2021-01-05 14:01 UTC #3 lore raw

In reply to Andy Shevchenko

Quoted 4 lines · Andy Shevchenko

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

author

2021-01-05 14:38 UTC #4 lore raw

Quoted 12 lines · Dan Carpenter

Quoted 4 lines · Andy Shevchenko
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".

Quoted 5 lines

	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!

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dan Carpenter

2021-01-05 16:07 UTC #5 lore raw

Quoted 13 lines · Andy Shevchenko

Quoted 12 lines · Dan Carpenter
Quoted 4 lines · Andy Shevchenko
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".

Quoted 7 lines

Quoted 5 lines
	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