netdev - Re: [RFC PATCH net-next v2 01/15] bpf: BPF support for socket ops

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: <EFA12996-9C47-444F-9E09-B87F673DCCB0@fb.com>
Date:   Sat, 17 Jun 2017 21:48:47 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        netdev <netdev@...r.kernel.org>
CC:     Kernel Team <Kernel-team@...com>, Blake Matheny <bmatheny@...com>,
        "Alexei Starovoitov" <ast@...com>,
        David Ahern <dsa@...ulusnetworks.com>
Subject: Re: [RFC PATCH net-next v2 01/15] bpf: BPF support for socket ops

On 6/16/17, 5:07 AM, "Daniel Borkmann" <daniel@...earbox.net> wrote:

    On 06/15/2017 10:08 PM, Lawrence Brakmo wrote:
    > Two new corresponding structs (one for the kernel one for the user/BPF
    > program):
    >
    > /* kernel version */
    > struct bpf_socket_ops_kern {
    >          struct sock *sk;
    > 	__u32  is_req_sock:1;
    >          __u32  op;
    >          union {
    >                  __u32 reply;
    >                  __u32 replylong[4];
    >          };
    > };
    >
    > /* user version */
    > struct bpf_socket_ops {
    >          __u32 op;
    >          union {
    >                  __u32 reply;
    >                  __u32 replylong[4];
    >          };
    >          __u32 family;
    >          __u32 remote_ip4;
    >          __u32 local_ip4;
    >          __u32 remote_ip6[4];
    >          __u32 local_ip6[4];
    >          __u32 remote_port;
    >          __u32 local_port;
    > };
    
    Above and ...
    
    struct bpf_sock {
    	__u32 bound_dev_if;
    	__u32 family;
    	__u32 type;
    	__u32 protocol;
    };
    
    ... would result in two BPF sock user versions. It's okayish, but
    given struct bpf_sock is quite generic, couldn't we merge the members
    from struct bpf_socket_ops into struct bpf_sock instead?
    
    Idea would be that sock_filter_is_valid_access() for cgroups would
    then check off < 0 || off + size > offsetofend(struct bpf_sock, protocol)
    to disallow new members, and your socket_ops_is_valid_access() could
    allow and xlate the full range. The family member is already duplicate
    and the others could then be accessed from these kind of BPF progs as
    well, plus we have a single user representation similar as with __sk_buff
    that multiple types will use.
    
I am concerned that it could make usage more confusing. One type of 
sock program (cgroup) could only use a subset of the fields while the
other type (socket_ops) could use all (or a different subset). Then what
happens if there is a need to add a new field to cgroup type sock program?
In addition, in the near future I will have a patch to attach socket_ops
programs to cgroups.
I rather leave it as it is.