Skip to contentSkip to conversation

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
#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
#1 lore raw

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.

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

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

syzbot
#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

#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

author
#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
#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