netdev - Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190228173738.squ62xmpkjuvjwmd@kafai-mbp.dhcp.thefacebook.com>
Date:   Thu, 28 Feb 2019 17:37:40 +0000
From:   Martin Lau <kafai@...com>
To:     Lorenz Bauer <lmb@...udflare.com>
CC:     "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>
Subject: Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

On Thu, Feb 28, 2019 at 03:11:09PM +0000, Lorenz Bauer wrote:

> I've started working on this, and I've hit a snag with the reference
> tracking behaviour
> of bpf_tcp_sock. From what I can tell, the assumption is that a PTR_TO_TCP_SOCK
> doesn't need reference tracking, because its either skb->sk or a TCP listener.
> In the former case, the socket is refcounted via the sk_buff, in the
> latter we don't need
> to worry since the eBPF is called with the RCU read lock held.
> 
> However, non-listening sockets returned by bpf_sk_lookup_tcp, can be
> freed before the
> end of the eBPF program. Doing bpf_sk_lookup_tcp, bpf_tcp_sock,
> bpf_sk_release allows
> eBPF to gain a (read-only) reference to a freed socket. I've attached
> a patch with a testcase
> which illustrates this issue.
> 
> Is this the intended behaviour? If not, maybe it would be the easiest
> to make bpf_tcp_sock
> increase the refcount if !SOCK_RCU_FREE and require a corresponding
> bpf_sk_release?
Increase the refcount at runtime may be a too big hammer for this.
Let me think if it can be resolved within the verifier.

> That would simplify my work to add RET_PTR_TO_SOCK_COMMON as wel..
> 
> ---
>  tools/testing/selftests/bpf/verifier/sock.c | 23 +++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/verifier/sock.c
> b/tools/testing/selftests/bpf/verifier/sock.c
> index 0ddfdf76aba5..3307cca6bdd5 100644
> --- a/tools/testing/selftests/bpf/verifier/sock.c
> +++ b/tools/testing/selftests/bpf/verifier/sock.c
> @@ -382,3 +382,26 @@
>         .result = REJECT,
>         .errstr = "type=tcp_sock expected=sock",
>  },
> +{
> +       "use bpf_tcp_sock after bpf_sk_release",
> +       .insns = {
> +       BPF_SK_LOOKUP,
> +       BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
> +       BPF_EXIT_INSN(),
> +       BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> +       BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
> +       BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> +       BPF_EMIT_CALL(BPF_FUNC_sk_release),
> +       BPF_EXIT_INSN(),
> +       BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> +       BPF_EMIT_CALL(BPF_FUNC_sk_release),
> +       BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct
> bpf_tcp_sock, snd_cwnd)),
> +       BPF_EXIT_INSN(),
> +       },
> +       .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> +       .result = REJECT,
> +       .errstr = "bogus",
> +},
> --
> 2.19.1