netdev - Re: [PATCH bpf-next] selftests/xsk: fix double free

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Sep 2022 16:31:37 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Martin KaFai Lau <martin.lau@...ux.dev>
Cc:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        magnus.karlsson@...el.com, bjorn@...nel.org, ast@...nel.org,
        daniel@...earbox.net, netdev@...r.kernel.org,
        jonathan.lemon@...il.com, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next] selftests/xsk: fix double free

On Fri, Sep 30, 2022 at 9:52 AM Magnus Karlsson
<magnus.karlsson@...il.com> wrote:
>
> On Fri, Sep 30, 2022 at 2:52 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
> >
> > On 9/29/22 3:44 AM, Maciej Fijalkowski wrote:
> > > On Thu, Sep 29, 2022 at 11:01:33AM +0200, Magnus Karlsson wrote:
> > >> From: Magnus Karlsson <magnus.karlsson@...el.com>
> > >>
> > >> Fix a double free at exit of the test suite.
> > >>
> > >> Fixes: a693ff3ed561 ("selftests/xsk: Add support for executing tests on physical device")
> > >> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > >> ---
> > >>   tools/testing/selftests/bpf/xskxceiver.c | 3 ---
> > >>   1 file changed, 3 deletions(-)
> > >>
> > >> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > >> index ef33309bbe49..d1a5f3218c34 100644
> > >> --- a/tools/testing/selftests/bpf/xskxceiver.c
> > >> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > >> @@ -1953,9 +1953,6 @@ int main(int argc, char **argv)
> > >>
> > >>      pkt_stream_delete(tx_pkt_stream_default);
> > >>      pkt_stream_delete(rx_pkt_stream_default);
> > >> -    free(ifobj_rx->umem);
> > >> -    if (!ifobj_tx->shared_umem)
> > shared_umem means ifobj_rx->umem and ifobj_tx->umem are the same?  No special
> > handling is needed and ifobject_delete() will handle it?
>
> You are correct, we will still have a double free in that case. Thanks
> for spotting. Will send a v2.

Sorry, but I have to take my statement back. The v1 is actually
correct. The umem structure is unconditionally allocated in
ifobject_create(). Later when setting up the shared_umem, the
information from one of them is copied over to the other, except for
some information that is changed for the second umem structure. So the
v1 still stands.

> > >> -            free(ifobj_tx->umem);
> > >>      ifobject_delete(ifobj_tx);
> > >>      ifobject_delete(ifobj_rx);
> > >
> > > So basically we free this inside ifobject_delete().
> >