netdev - Re: [PATCH v4 bpf 0/5] Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk

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

Date:   Thu, 14 Mar 2019 19:02:45 +0000
From:   Alexei Starovoitov <ast@...com>
To:     Martin Lau <kafai@...com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>,
        Lorenz Bauer <lmb@...udflare.com>
Subject: Re: [PATCH v4 bpf 0/5] Fix bpf_tcp_sock and bpf_sk_fullsock issue
 related to bpf_sk_release

On 3/13/19 10:38 PM, Martin Lau wrote:
> On Wed, Mar 13, 2019 at 12:20:27PM -0700, Alexei Starovoitov wrote:
>> On Tue, Mar 12, 2019 at 10:23:01AM -0700, Martin KaFai Lau wrote:
>>> This set addresses issue about accessing invalid
>>> ptr returned from bpf_tcp_sock() and bpf_sk_fullsock()
>>> after bpf_sk_release().
>>>
>>> v4:
>>> - Tried the one "id" approach.  It does not work well and the reason is in
>>>    the Patch 1 commit message.
>>> - Rename refcount_id to ref_obj_id.
>>> - With ref_obj_id, resetting reg->id to 0 is fine in mark_ptr_or_null_reg()
>>>    because ref_obj_id is passed to release_reference() instead of reg->id.
>>> - Also reset reg->ref_obj_id in mark_ptr_or_null_reg() when is_null == true
>>> - sk_to_full_sk() is removed from bpf_sk_fullsock() and bpf_tcp_sock().
>>> - bpf_get_listener_sock() is added to do sk_to_full_sk() in Patch 2.
>>> - If tp is from bpf_tcp_sock(sk) and sk is a refcounted ptr,
>>>    bpf_sk_release(tp) is also allowed.
>>
>> Thanks for the hard work on this fix.
>> I applied the set to bpf tree.
> Thanks for reviewing.
> 
>>
>> I have a small question regarding patch 2:
>> +BPF_CALL_1(bpf_get_listener_sock, struct sock *, sk)
>> +{
>> +       sk = sk_to_full_sk(sk);
>> +
>> +       if (sk->sk_state == TCP_LISTEN && sock_flag(sk, SOCK_RCU_FREE))
>> +               return (unsigned long)sk;
>> +
>> +       return (unsigned long)NULL;
>> +}
>>
>> My understanding that sk->sk_state == TCP_LISTEN is enough for correctness
>> and additional sock_flag(sk, SOCK_RCU_FREE) check is to prevent
>> silent breakage in case listener socks will be re-implemented without rcu_free?
>> In such case should we add warn_on_once(!sock_flag(sk, SOCK_RCU_FREE)) too?
>> I mean in the very unlikely scenario that kernel will have such drastic
>> implementation change the bpf progs will not work correctly because NULL
>> is returned, but it will be harder for humans to debug?
>> I think it's also ok without warn, since such huge re-implemenation will
>> require all sorts of efforts, so highly unlikely we will miss this spot.
>> warn_on_once feels like too much paranoia. May be just a comment ?
>> Or I'm overthinking?
> No.  It is a valid question.  Sorry that I missed details in this patch.
> 
> Here is what I had thought about in the patch:
> 1. inet_reqsk(sk)->rsk_listener is only set for TCP (and dccp).
>     Hence, sk_to_full_sk(sk) is only useful for TCP (and dccp).
> 
> 2. The TCP_LISTEN sock has SOCK_RCU_FREE set when it is added
>     to the listening_hash in __inet_hash().  It is done for
>     TCP (and dccp).
> 
> 3. However, not all TCP_LISTEN socket is in SOCK_RCU_FREE.
>     e.g. af_unix.c.  It may be accessible through skb->sk.
>     If WARN_ON_ONCE() was used, it would be a false alarm.
> 
>     For those non TCP sk, sk_to_full_sk(sk) is a no-op.
>     If its passed-in sk is not already in TCP_LISTEN,
>     bpf_get_listener_sock() has to return NULL anyway.
> 
>     It is not ideal for those sk because this helper
>     still returns NULL if its passed-in sk is already TCP_LISTEN
>     because of the sock_flag(sk, SOCK_RCU_FREE) check.
> 
>     On the other hand, there is sk->sk_state for the bpf_prog
>     to check first before calling this helper.

all of the above makes sense to me. thanks for explaining.
With that I think it's best to leave it as-is.
I don't see how any of the below options make it better.

> I have re-thought about (3).  How about this:
> 1. Limit this helper to "sk->sk_protocol == IPPROTO_TCP".
>     Return NULL for others because this helper cannot
>     do anything extra for those sk anyway.

protocol is already in bpf_sock and prog can check it if necessary.

> 2. Rename this helper to bpf_get_tcp_listener_sock().  It still returns
>     "struct bpf_sock *" because I think the send_cwnd/srtt_us/rtt_min/...
>     of the listener's "struct bpf_tcp_sock *" is not very useful.
> 
>     If needed, bpf_tcp_sock() can be used to get "struct bpf_tcp_sock *".
>     
> 3. Add WARN_ON_ONCE() on the SOCK_RCU_FREE flag.

not needed as you explained it will be false alarm.