netdev - Re: [PATCH bpf] xsk: fix memory leak for failed bind

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]

Message-ID: <CAJ8uoz0Mhmc7oW=T54p-bE-fa=m6YR69UVuVFhFWgATDVk1U_g@mail.gmail.com>
Date:   Mon, 14 Dec 2020 13:33:26 +0100
From:   "magnus.karlsson" <magnus.karlsson@...il.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        bpf <bpf@...r.kernel.org>,
        syzbot+cfa88ddd0655afa88763@...kaller.appspotmail.com
Subject: Re: [PATCH bpf] xsk: fix memory leak for failed bind

On Mon, Dec 14, 2020 at 12:33 PM Magnus Karlsson
<magnus.karlsson@...il.com> wrote:
>
> On Mon, Dec 14, 2020 at 12:19 PM Maciej Fijalkowski
> <maciej.fijalkowski@...el.com> wrote:
> >
> > On Mon, Dec 14, 2020 at 09:51:27AM +0100, Magnus Karlsson wrote:
> > > From: Magnus Karlsson <magnus.karlsson@...el.com>
> > >
> > > Fix a possible memory leak when a bind of an AF_XDP socket fails. When
> > > the fill and completion rings are created, they are tied to the
> > > socket. But when the buffer pool is later created at bind time, the
> > > ownership of these two rings are transferred to the buffer pool as
> > > they might be shared between sockets (and the buffer pool cannot be
> > > created until we know what we are binding to). So, before the buffer
> > > pool is created, these two rings are cleaned up with the socket, and
> > > after they have been transferred they are cleaned up together with
> > > the buffer pool.
> > >
> > > The problem is that ownership was transferred before it was absolutely
> > > certain that the buffer pool could be created and initialized
> > > correctly and when one of these errors occurred, the fill and
> > > completion rings did neither belong to the socket nor the pool and
> > > where therefore leaked. Solve this by moving the ownership transfer
> > > to the point where the buffer pool has been completely set up and
> > > there is no way it can fail.
> > >
> > > Fixes: 7361f9c3d719 ("xsk: Move fill and completion rings to buffer pool")
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > > Reported-by: syzbot+cfa88ddd0655afa88763@...kaller.appspotmail.com
> > > ---
> > >  net/xdp/xsk.c           | 4 ++++
> > >  net/xdp/xsk_buff_pool.c | 2 --
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index 62504471fd20..189cfbbcccc0 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -772,6 +772,10 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> > >               }
> > >       }
> > >
> > > +     /* FQ and CQ are now owned by the buffer pool and cleaned up with it. */
> > > +     xs->fq_tmp = NULL;
> > > +     xs->cq_tmp = NULL;
> > > +
> > >       xs->dev = dev;
> > >       xs->zc = xs->umem->zc;
> > >       xs->queue_id = qid;
> > > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > > index d5adeee9d5d9..46c2ae7d91d1 100644
> > > --- a/net/xdp/xsk_buff_pool.c
> > > +++ b/net/xdp/xsk_buff_pool.c
> > > @@ -75,8 +75,6 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
> > >
> > >       pool->fq = xs->fq_tmp;
> > >       pool->cq = xs->cq_tmp;
> > > -     xs->fq_tmp = NULL;
> > > -     xs->cq_tmp = NULL;
> >
> > Given this change, are there any circumstances that we could hit
> > xsk_release with xs->{f,c}q_tmp != NULL ?
>
> Yes, if the user has not registered any fill or completion ring and
> the socket is torn down.

Sorry Maciej. I answered the inverse of your question, i.e. == NULL.
For != NULL answer:

Yes, this is possible if the user registers a fill ring and/or
completion ring but does not bind and then closes the socket.

> > >
> > >       for (i = 0; i < pool->free_heads_cnt; i++) {
> > >               xskb = &pool->heads[i];
> > >
> > > base-commit: d9838b1d39283c1200c13f9076474c7624b8ec34
> > > --
> > > 2.29.0
> > >