netdev - Re: [Patch net v2] genetlink: fix memory leaks in genl_family_rcv_msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAM_iQpVU14ky_L7PqhyB3_OSPpbY5ZKLn=GonLU88GcN5fEoHw@mail.gmail.com>
Date:   Wed, 10 Jun 2020 21:14:10 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        syzbot+21f04f481f449c8db840@...kaller.appspotmail.com,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Florian Westphal <fw@...len.de>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Jiri Pirko <jiri@...lanox.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Shaochun Chen <cscnull@...il.com>
Subject: Re: [Patch net v2] genetlink: fix memory leaks in genl_family_rcv_msg_dumpit()

On Wed, Jun 10, 2020 at 7:27 AM Ido Schimmel <idosch@...sch.org> wrote:
> > @@ -548,7 +591,7 @@ static int genl_lock_done(struct netlink_callback *cb)
> >               rc = ops->done(cb);
> >               genl_unlock();
> >       }
> > -     genl_family_rcv_msg_attrs_free(info->family, info->attrs, true);
> > +     genl_family_rcv_msg_attrs_free(info->family, info->attrs, false);
>
> Cong,
>
> This seems to result in a memory leak because 'info->attrs' is never
> freed in the non-parallel case.
>
> Both the parallel and non-parallel code paths call genl_start() which
> allocates the array, but the latter calls genl_lock_done() as its done()
> callback which never frees it.

Good catch! Looks like I should just revert the above chunk. The
last parameter of genl_family_rcv_msg_attrs_free() is just confusing,
genl_lock_done() is clearly not parallel at all..

I will take a deeper look and send out a patch tomorrow.

Thanks!