lkml.org 
[lkml]   [2022]   [Aug]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v2] media: dvb-usb: fix memory leak in dvb_usb_adapter_init()
    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

    \
     
     \ /
      Last update: 2022-08-05 08:40    [W:4.246 / U:0.744 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site