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 09:09:54 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jonathan Maxwell <jmaxwell37@...il.com>,
        Martin KaFai Lau <kafai@...com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, pabeni@...hat.com,
        Antoine Tenart <atenart@...nel.org>, cutaylor-pub@...oo.com,
        alexei.starovoitov@...il.com, joe@...ium.io, i@....io,
        bpf@...r.kernel.org
Subject: Re: [PATCH net] net: bpf: fix request_sock leak in filter.c

On 6/10/22 2:45 AM, Jonathan Maxwell wrote:
> On Fri, Jun 10, 2022 at 10:36 AM Martin KaFai Lau <kafai@...com> wrote:
>>
>> On Thu, Jun 09, 2022 at 05:17:47PM -0700, Martin KaFai Lau wrote:
>>> On Thu, Jun 09, 2022 at 10:29:15PM +0200, Daniel Borkmann wrote:
>>>> On 6/9/22 3:18 AM, Jon Maxwell wrote:
>>>>> 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
>>> Great catch and debug indeed!
>>>
>>>>> 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")
>>> Instead of the above commits, I think this dated back to
>>> 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
>>
>> Since this is more bpf specific, I think it could go to the bpf tree.
>> In v2, please cc bpf@...r.kernel.org and tag it with 'PATCH v2 bpf'.
> 
> Okay thanks will do.
> 
> Daniel, are you okay with omitting 'if (unlikely...) { WARN_ONCE(...); }'?
> 
> If so I'll stick to the rest of the logic of your suggestion and omit that
> check in v1.

Ok, works for me, see also my other reply that we should at least mention it in
the commit log.

Thanks!
Daniel