netdev - Re: [PATCH net] net: bpf: fix request

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, 10 Jun 2022 10:49:13 +1000
From:   Jonathan Maxwell <jmaxwell37@...il.com>
To:     Antoine Tenart <atenart@...nel.org>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, pabeni@...hat.com,
        cutaylor-pub@...oo.com
Subject: Re: [PATCH net] net: bpf: fix request_sock leak in filter.c

On Thu, Jun 9, 2022 at 11:35 PM Antoine Tenart <atenart@...nel.org> wrote:
>
> Hi Jon,
>
> Quoting Jon Maxwell (2022-06-09 03:18:44)
> > A customer reported a request_socket leak in a Calico cloud environment. We
> > found that a BPF program was doing a socket lookup with takes a refcnt on
> > the socket and that it was finding the request_socket but returning the parent
> > LISTEN socket via sk_to_full_sk() without decrementing the child request socket
> > 1st, resulting in request_sock slab object leak. This patch retains the
> > existing behaviour of returning full socks to the caller but it also decrements
> > the child request_socket if one is present before doing so to prevent the leak.
> >
> > Thanks to Curtis Taylor for all the help in diagnosing and testing this. And
> > thanks to Antoine Tenart for the reproducer and patch input.
> >
> > Fixes: f7355a6c0497 bpf: ("Check sk_fullsock() before returning from bpf_sk_lookup()")
> > Fixes: edbf8c01de5a bpf: ("add skc_lookup_tcp helper")
>
> "bpf:" should be inside the parenthesis in the two above lines.
>
> Isn't the issue from before edbf8c01de5a for bpf_sk_lookup? Looking at a
> 5.1 kernel[1], __bpf_sk_lookup was called and also did the full socket
> translation[2]. bpf_sk_release would not be called on the original
> socket when that happens.
>
> [1] https://elixir.bootlin.com/linux/v5.1/source/net/core/filter.c#L5204
> [2] https://elixir.bootlin.com/linux/v5.1/source/net/core/filter.c#L5198
>
> > Tested-by: Curtis Taylor <cutaylor-pub@...oo.com>
> > Co-developed-by: Antoine Tenart <atenart@...nel.org>
> > Signed-off-by:: Antoine Tenart <atenart@...nel.org>
>
> Please remove the extra ':'.
>

Sure will correct those typos in v1.

Regards

Jon

> Thanks!
> Antoine
>
> > Signed-off-by: Jon Maxwell <jmaxwell37@...il.com>
> > ---
> >  net/core/filter.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 2e32cee2c469..e3c04ae7381f 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6202,13 +6202,17 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  {
> >         struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> >                                            ifindex, proto, netns_id, flags);
> > +       struct sock *sk1 = sk;
> >
> >         if (sk) {
> >                 sk = sk_to_full_sk(sk);
> > -               if (!sk_fullsock(sk)) {
> > -                       sock_gen_put(sk);
> > +               /* sk_to_full_sk() may return (sk)->rsk_listener, so make sure the original sk1
> > +                * sock refcnt is decremented to prevent a request_sock leak.
> > +                */
> > +               if (!sk_fullsock(sk1))
> > +                       sock_gen_put(sk1);
> > +               if (!sk_fullsock(sk))
> >                         return NULL;
> > -               }
> >         }
> >
> >         return sk;
> > @@ -6239,13 +6243,17 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  {
> >         struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
> >                                          flags);
> > +       struct sock *sk1 = sk;
> >
> >         if (sk) {
> >                 sk = sk_to_full_sk(sk);
> > -               if (!sk_fullsock(sk)) {
> > -                       sock_gen_put(sk);
> > +               /* sk_to_full_sk() may return (sk)->rsk_listener, so make sure the original sk1
> > +                * sock refcnt is decremented to prevent a request_sock leak.
> > +                */
> > +               if (!sk_fullsock(sk1))
> > +                       sock_gen_put(sk1);
> > +               if (!sk_fullsock(sk))
> >                         return NULL;
> > -               }
> >         }
> >
> >         return sk;
> > --
> > 2.31.1
> >