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

Date:   Wed, 13 Mar 2019 12:20:27 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     netdev@...r.kernel.org, Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>, 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 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.

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?
I guess I'm fine with the code as-is.