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:   Thu, 09 Jun 2022 15:35:22 +0200
From:   Antoine Tenart <atenart@...nel.org>
To:     Jon Maxwell <jmaxwell37@...il.com>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, cutaylor-pub@...oo.com,
        Jon Maxwell <jmaxwell37@...il.com>
Subject: Re: [PATCH net] net: bpf: fix request_sock leak in filter.c

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 ':'.

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
>