netdev - Re: [PATCH RFC bpf-next 1/6] bpf: Hooks for sys

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]

Message-ID: <136ee044-0cf7-a8d7-e6d2-3c4c362118e4@fb.com>
Date:   Wed, 14 Mar 2018 17:29:07 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     Alexei Starovoitov <ast@...nel.org>, <davem@...emloft.net>,
        <netdev@...r.kernel.org>, <kernel-team@...com>
Subject: Re: [PATCH RFC bpf-next 1/6] bpf: Hooks for sys_bind

On 3/14/18 4:27 PM, Daniel Borkmann wrote:
> On 03/14/2018 07:11 PM, Alexei Starovoitov wrote:
>> On Wed, Mar 14, 2018 at 03:37:01PM +0100, Daniel Borkmann wrote:
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -133,6 +133,8 @@ enum bpf_prog_type {
>>>>  	BPF_PROG_TYPE_SOCK_OPS,
>>>>  	BPF_PROG_TYPE_SK_SKB,
>>>>  	BPF_PROG_TYPE_CGROUP_DEVICE,
>>>> +	BPF_PROG_TYPE_CGROUP_INET4_BIND,
>>>> +	BPF_PROG_TYPE_CGROUP_INET6_BIND,
>>>
>>> Could those all be merged into BPF_PROG_TYPE_SOCK_OPS? I'm slowly getting
>>> confused by the many sock_*/sk_* prog types we have. The attach hook could
>>> still be something like BPF_CGROUP_BIND/BPF_CGROUP_CONNECT. Potentially
>>> storing some prog-type specific void *private_data in prog's aux during
>>> verification could be a way (similarly as you mention) which can later be
>>> retrieved at attach time to reject with -ENOTSUPP or such.
>>
>> that's exacly what I mentioned in the cover letter,
>> but we need to extend attach cmd with verifier-like log_buf+log_size.
>> since simple enotsupp will be impossible to debug.
>
> Hmm, adding verifier-like log_buf + log_size feels a bit of a kludge just
> for this case, but it's the usual problem where getting a std error code
> is like looking into a crystal ball to figure where it's coming from. I'd see
> only couple of other alternatives: distinct error code like ENAVAIL for such
> mismatch, or telling the verifier upfront where this is going to be attached
> to - same as we do with the dev for offloading or as you did with splitting
> the prog types or some sort of notion of sub-prog types; or having a query
> API that returns possible/compatible attach types for this loaded prog via
> e.g. bpf_prog_get_info_by_fd() that loader can precheck or check when error
> occurs. All nothing really nice, though.

query after loading isn't great, since possible attach combinations
will be too high for human to understand,
but I like "passing attach_type into prog_load" idea.
That should work and it fits existing prog_ifindex too.
So we'll add '__u32 attach_type' to prog_load cmd.
elf loader would still need to parse section name to
figure out prog type and attach type.
Something like:
SEC("sock_addr/bind_v4") my_prog(struct bpf_sock_addr *ctx)
SEC("sock_addr/connect_v6") my_prog(struct bpf_sock_addr *ctx)
We still need new prog type for bind_v4/bind_v6/connect_v4/connect_v6
hooks with distinct 'struct bpf_sock_addr' context,
since the prog is accessing both sockaddr and sock.
Adding user_ip4, user_ip6 fields to 'struct bpf_sock_ops'
is doable, but it would be too confusing to users, so imo that's
not a good option.

For post-bind hook we probably can reuse 'struct bpf_sock_ops'
and BPF_PROG_TYPE_SOCK_OPS, since there only sock is the context.

> Making verifier-like log_buf + log_size generic meaning not just for the case
> of BPF_PROG_ATTACH specifically might be yet another option, meaning you'd
> have a new BPF_GET_ERROR command to pick up the log for the last failed bpf(2)
> cmd, but either that might require adding a BPF related pointer to task struct
> for this or any other future BPF feature (maybe not really an option); or to
> have some sort of bpf cmd to config and obtain an fd for error queue/log once,
> where this can then be retrieved from and used for all cmds generically.

I don't think we want to hold on to error logs in the kernel,
since user may not query it right away or ever.
verifier log is freed right after prog_load cmd is done.

> Seems like it would potentially be on top of that, plus having an option to
> attach from within the app instead of orchestrator.

right. let's worry about it as potential next step.