netdev - Re: [PATCH bpf-next v4 1/3] bpf: support input __sk_buff context in BPF_PROG_TEST

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]

Message-ID: <3e26d75a-da10-9ae2-12f8-893f334fa1e9@iogearbox.net>
Date:   Fri, 12 Apr 2019 00:23:46 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Cc:     davem@...emloft.net, ast@...nel.org, Martin Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v4 1/3] bpf: support input __sk_buff context in
 BPF_PROG_TEST_RUN

Hey Stanislav,

On 04/09/2019 08:49 PM, Stanislav Fomichev wrote:
> Add new set of arguments to bpf_attr for BPF_PROG_TEST_RUN:
> * ctx_in/ctx_size_in - input context
> * ctx_out/ctx_size_out - output context
> 
> The intended use case is to pass some meta data to the test runs that
> operate on skb (this has being brought up on recent LPC).
> 
> For programs that use bpf_prog_test_run_skb, support __sk_buff input and
> output. Initially, from input __sk_buff, copy _only_ cb and priority into
> skb, all other non-zero fields are prohibited (with EINVAL).
> If the user has set ctx_out/ctx_size_out, copy the potentially modified
> __sk_buff back to the userspace.
> 
> We require all fields of input __sk_buff except the ones we explicitly
> support to be set to zero. The expectation is that in the future we might
> add support for more fields and we want to fail explicitly if the user
> runs the program on the kernel where we don't yet support them.
> 
> The API is intentionally vague (i.e. we don't explicitly add __sk_buff
> to bpf_attr, but ctx_in) to potentially let other test_run types use
> this interface in the future (this can be xdp_md for xdp types for
> example).
> 
> v4:
>   * don't copy more than allowed in bpf_ctx_init [Martin]
> 
> v3:
>   * handle case where ctx_in is NULL, but ctx_out is not [Martin]
>   * convert size==0 checks to ptr==NULL checks and add some extra ptr
>     checks [Martin]
> 
> v2:
>   * Addressed comments from Martin Lau
> 
> Cc: Martin Lau <kafai@...com>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>

This still has a bug in that we need to reject !bpf_prog_test_run_skb() cases,
since they are not handled by your set. So for e.g. XDP, flow dissector progs,
we need to error out if ctx is set such that it can be safely extended in future.
Please follow up.

Thanks,
Daniel