linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init()
@ 2022-07-24 21:41 Mazin Al Haddad
  2022-08-04 23:07 ` Mazin Al Haddad
  2022-08-05  6:39 ` Lukas Bulwahn
  0 siblings, 2 replies; 4+ messages in thread
From: Mazin Al Haddad @ 2022-07-24 21:41 UTC (permalink / raw)
  To: linux-media
  Cc: syzbot+f66dd31987e6740657be, linux-kernel, mchehab,
	linux-kernel-mentees, Mazin Al Haddad

Fix a memory leak in dvb_usb_adapter_init() reported by syzkaller. The 
problem is due to the error path exiting before incrementing 
num_adapters_initalized, which is used as a reference counter to free 
adapter's private data. There are multiple error paths that 
dvb_usb_adapter_init() can exit from before incrementing the counter, 
which lead to a memory leak as the current iteration is not accounted for.
Fix this by freeing the current iteration's adap->priv in each of the 
error paths.

Syz Report:
BUG: memory leak
unreferenced object 0xffff8881172f1a00 (size 512):
  comm "kworker/0:2", pid 139, jiffies 4294994873 (age 10.960s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff844af012>] dvb_usb_adapter_init drivers/media/usb/dvb-usb/dvb-usb-init.c:75 [inline]
    [<ffffffff844af012>] dvb_usb_init drivers/media/usb/dvb-usb/dvb-usb-init.c:184 [inline]
    [<ffffffff844af012>] dvb_usb_device_init.cold+0x4e5/0x79e drivers/media/usb/dvb-usb/dvb-usb-init.c:308
    [<ffffffff830db21d>] dib0700_probe+0x8d/0x1b0 drivers/media/usb/dvb-usb/dib0700_core.c:883
    [<ffffffff82d3fdc7>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff8274ab37>] call_driver_probe drivers/base/dd.c:542 [inline]
    [<ffffffff8274ab37>] really_probe.part.0+0xe7/0x310 drivers/base/dd.c:621
    [<ffffffff8274ae6c>] really_probe drivers/base/dd.c:583 [inline]
    [<ffffffff8274ae6c>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:752
    [<ffffffff8274af6a>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:782
    [<ffffffff8274b786>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:899
    [<ffffffff82747c87>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff8274b352>] __device_attach+0x122/0x260 drivers/base/dd.c:970
    [<ffffffff827498f6>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
    [<ffffffff82745cdb>] device_add+0x5fb/0xdf0 drivers/base/core.c:3405
    [<ffffffff82d3d202>] usb_set_configuration+0x8f2/0xb80 drivers/usb/core/message.c:2170
    [<ffffffff82d4dbfc>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238
    [<ffffffff82d3f49c>] usb_probe_device+0x5c/0x140 drivers/usb/core/driver.c:293
    [<ffffffff8274ab37>] call_driver_probe drivers/base/dd.c:542 [inline]
    [<ffffffff8274ab37>] really_probe.part.0+0xe7/0x310 drivers/base/dd.c:621
    [<ffffffff8274ae6c>] really_probe drivers/base/dd.c:583 [inline]
    [<ffffffff8274ae6c>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:752

Link: https://syzkaller.appspot.com/bug?id=4d54f8bf7b98eecf6cd76ed5aaea883c5d9e502a
Reported-by: syzbot+f66dd31987e6740657be@syzkaller.appspotmail.com
Signed-off-by: Mazin Al Haddad <mazinalhaddad05@gmail.com>
---

Changes in v2:
- Remove variable that is used to refcount and instead free current
  iteration private data.

 drivers/media/usb/dvb-usb/dvb-usb-init.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
index 61439c8f33ca..69520d7ca25d 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
@@ -80,16 +80,22 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs)
 		}
 
 		ret = dvb_usb_adapter_stream_init(adap);
-		if (ret)
+		if (ret) {
+			kfree(adap->priv);
 			return ret;
+		}
 
 		ret = dvb_usb_adapter_dvb_init(adap, adapter_nrs);
-		if (ret)
+		if (ret) {
+			kfree(adap->priv);
 			goto dvb_init_err;
+		}
 
 		ret = dvb_usb_adapter_frontend_init(adap);
-		if (ret)
+		if (ret) {
+			kfree(adap->priv);
 			goto frontend_init_err;
+		}
 
 		/* use exclusive FE lock if there is multiple shared FEs */
 		if (adap->fe_adap[1].fe)
@@ -112,6 +118,7 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs)
 
 frontend_init_err:
 	dvb_usb_adapter_dvb_exit(adap);
+	return ret;
 dvb_init_err:
 	dvb_usb_adapter_stream_exit(adap);
 	return ret;
-- 
2.37.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init()
  2022-07-24 21:41 [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init() Mazin Al Haddad
@ 2022-08-04 23:07 ` Mazin Al Haddad
  2022-08-05  4:21   ` Greg KH
  2022-08-05  6:39 ` Lukas Bulwahn
  1 sibling, 1 reply; 4+ messages in thread
From: Mazin Al Haddad @ 2022-08-04 23:07 UTC (permalink / raw)
  To: Mazin Al Haddad, linux-media
  Cc: syzbot+f66dd31987e6740657be, mchehab, linux-kernel-mentees, linux-kernel

Hi, did anyone have the opportunity to review this? Any feedback would
be appreciated.

Thanks for your time!
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init()
  2022-08-04 23:07 ` Mazin Al Haddad
@ 2022-08-05  4:21   ` Greg KH
  0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2022-08-05  4:21 UTC (permalink / raw)
  To: Mazin Al Haddad
  Cc: linux-kernel-mentees, mchehab, syzbot+f66dd31987e6740657be,
	linux-kernel, linux-media

On Fri, Aug 05, 2022 at 02:07:51AM +0300, Mazin Al Haddad wrote:
> Hi, did anyone have the opportunity to review this?

What is "this"?  There is no context here.

Also please realize this is the middle of the merge window, maintainers
can not accept anything until after -rc1 is out.  Normally you should
wait at least 2 weeks before resubmitting if you have not gotten a
response, but during the merge window, you should wait a bit longer.

thanks,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init()
  2022-07-24 21:41 [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init() Mazin Al Haddad
  2022-08-04 23:07 ` Mazin Al Haddad
@ 2022-08-05  6:39 ` Lukas Bulwahn
  1 sibling, 0 replies; 4+ messages in thread
From: Lukas Bulwahn @ 2022-08-05  6:39 UTC (permalink / raw)
  To: Mazin Al Haddad
  Cc: linux-kernel-mentees, Mauro Carvalho Chehab,
	syzbot+f66dd31987e6740657be, Linux Kernel Mailing List,
	Linux Media Mailing List

On Sun, Jul 24, 2022 at 11:42 PM Mazin Al Haddad
<mazinalhaddad05@gmail.com> wrote:
>
> Fix a memory leak in dvb_usb_adapter_init() reported by syzkaller. The
> problem is due to the error path exiting before incrementing
> num_adapters_initalized, which is used as a reference counter to free
> adapter's private data. There are multiple error paths that
> dvb_usb_adapter_init() can exit from before incrementing the counter,
> which lead to a memory leak as the current iteration is not accounted for.
> Fix this by freeing the current iteration's adap->priv in each of the
> error paths.
>
> Syz Report:
> BUG: memory leak
> unreferenced object 0xffff8881172f1a00 (size 512):
>   comm "kworker/0:2", pid 139, jiffies 4294994873 (age 10.960s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff844af012>] dvb_usb_adapter_init drivers/media/usb/dvb-usb/dvb-usb-init.c:75 [inline]
>     [<ffffffff844af012>] dvb_usb_init drivers/media/usb/dvb-usb/dvb-usb-init.c:184 [inline]
>     [<ffffffff844af012>] dvb_usb_device_init.cold+0x4e5/0x79e drivers/media/usb/dvb-usb/dvb-usb-init.c:308
>     [<ffffffff830db21d>] dib0700_probe+0x8d/0x1b0 drivers/media/usb/dvb-usb/dib0700_core.c:883
>     [<ffffffff82d3fdc7>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
>     [<ffffffff8274ab37>] call_driver_probe drivers/base/dd.c:542 [inline]
>     [<ffffffff8274ab37>] really_probe.part.0+0xe7/0x310 drivers/base/dd.c:621
>     [<ffffffff8274ae6c>] really_probe drivers/base/dd.c:583 [inline]
>     [<ffffffff8274ae6c>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:752
>     [<ffffffff8274af6a>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:782
>     [<ffffffff8274b786>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:899
>     [<ffffffff82747c87>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
>     [<ffffffff8274b352>] __device_attach+0x122/0x260 drivers/base/dd.c:970
>     [<ffffffff827498f6>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
>     [<ffffffff82745cdb>] device_add+0x5fb/0xdf0 drivers/base/core.c:3405
>     [<ffffffff82d3d202>] usb_set_configuration+0x8f2/0xb80 drivers/usb/core/message.c:2170
>     [<ffffffff82d4dbfc>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238
>     [<ffffffff82d3f49c>] usb_probe_device+0x5c/0x140 drivers/usb/core/driver.c:293
>     [<ffffffff8274ab37>] call_driver_probe drivers/base/dd.c:542 [inline]
>     [<ffffffff8274ab37>] really_probe.part.0+0xe7/0x310 drivers/base/dd.c:621
>     [<ffffffff8274ae6c>] really_probe drivers/base/dd.c:583 [inline]
>     [<ffffffff8274ae6c>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:752
>
> Link: https://syzkaller.appspot.com/bug?id=4d54f8bf7b98eecf6cd76ed5aaea883c5d9e502a
> Reported-by: syzbot+f66dd31987e6740657be@syzkaller.appspotmail.com
> Signed-off-by: Mazin Al Haddad <mazinalhaddad05@gmail.com>
> ---
>
> Changes in v2:
> - Remove variable that is used to refcount and instead free current
>   iteration private data.
>
>  drivers/media/usb/dvb-usb/dvb-usb-init.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> index 61439c8f33ca..69520d7ca25d 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> @@ -80,16 +80,22 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs)
>                 }
>
>                 ret = dvb_usb_adapter_stream_init(adap);
> -               if (ret)
> +               if (ret) {
> +                       kfree(adap->priv);
>                         return ret;
> +               }
>
>                 ret = dvb_usb_adapter_dvb_init(adap, adapter_nrs);
> -               if (ret)
> +               if (ret) {
> +                       kfree(adap->priv);
>                         goto dvb_init_err;
> +               }
>
>                 ret = dvb_usb_adapter_frontend_init(adap);
> -               if (ret)
> +               if (ret) {
> +                       kfree(adap->priv);
>                         goto frontend_init_err;
> +               }
>
>                 /* use exclusive FE lock if there is multiple shared FEs */
>                 if (adap->fe_adap[1].fe)
> @@ -112,6 +118,7 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs)
>
>  frontend_init_err:
>         dvb_usb_adapter_dvb_exit(adap);
> +       return ret;
>  dvb_init_err:
>         dvb_usb_adapter_stream_exit(adap);
>         return ret;

Mazin,

I did not review if this patch is right semantically, but concerning
how to write and to modify error handling paths, there are a few
questions and potential improvements for your patch:

Before your patch,  dvb_usb_adapter_init() had three exit paths:

- an early return when dvb_usb_adapter_stream_init() fails
- a path to label dvb_init_err when dvb_usb_adapter_dvb_init() fails
- a path to label frontend_init_err when dvb_usb_adapter_stream_exit() fails

When dvb_usb_adapter_stream_exit() fails, the operations needed to
roll back dvb_usb_adapter_dvb_init() were also needed.
In other words, the execution path from frontend_init_err continues
into the operations of the label dvb_init_err and does not just
return.

With your patch, you changed this:

Why do you now not need to call dvb_usb_adapter_stream_exit() in the
frontend_init_err case?

Now, a simple syntactic and stylistic improvement to consider:

You would like to have a kfree on all three error paths, rather than
adding them three times you could just add them once. I have not
checked if kfree(...) needs to be called before
dvb_usb_adapter_dvb_exit() and dvb_usb_adapter_stream_exit(), or if it
is fine to just call it after. Usually, this patch below implements a
pretty standard pattern, though:

diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c
b/drivers/media/usb/dvb-usb/dvb-usb-init.c
index 61439c8f33ca..58eea8ab5477 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
@@ -81,7 +81,7 @@ static int dvb_usb_adapter_init(struct
dvb_usb_device *d, short *adapter_nrs)

                ret = dvb_usb_adapter_stream_init(adap);
                if (ret)
-                       return ret;
+                       goto stream_init_err;

                ret = dvb_usb_adapter_dvb_init(adap, adapter_nrs);
                if (ret)
@@ -114,6 +114,8 @@ static int dvb_usb_adapter_init(struct
dvb_usb_device *d, short *adapter_nrs)
        dvb_usb_adapter_dvb_exit(adap);
 dvb_init_err:
        dvb_usb_adapter_stream_exit(adap);
+stream_init_err:
+       kfree(adap->priv);
        return ret;
 }


I hope this helps a bit in your work.


Just another hint:

It might help if you can describe how somebody else (e.g., another
mentee) can trigger the memory leak without your patch and see it
being gone with your patch applied:

Provide the kernel config (better even just the fragment needed), the
link to a rootfs, the exact qemu command and the C reproducer.

Then ask other mentees to test before and after the application of
your patch and report their results. It is perfectly fine to ask
others in the community to help you---they will help if they know you
will help them in the future as well. The simpler the task and the
better the task needed for testing is described, the higher chances
that somebody may help here.

In the end that interaction among mentees will also convince the
maintainers (in this case: Mauro) that picking this patch into his
tree is the right thing to do.

Good luck.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-08-05  6:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-24 21:41 [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init() Mazin Al Haddad
2022-08-04 23:07 ` Mazin Al Haddad
2022-08-05  4:21   ` Greg KH
2022-08-05  6:39 ` Lukas Bulwahn

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