netdev - Re: [PATCH v2 bpf-next 3/9] 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: <c25dd3aa-bd1f-c219-8d9e-5bf94110e3c0@fb.com>
Date:   Thu, 29 Mar 2018 17:01:17 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrey Ignatov <rdna@...com>
CC:     <netdev@...r.kernel.org>, <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 3/9] bpf: Hooks for sys_bind

On 3/29/18 4:06 PM, Daniel Borkmann wrote:
> On 03/28/2018 05:41 AM, Alexei Starovoitov wrote:
> [...]
>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> index e8c7fad8c329..2dec266507dc 100644
>> --- a/net/ipv4/af_inet.c
>> +++ b/net/ipv4/af_inet.c
>> @@ -450,6 +450,13 @@ int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>>  	if (addr_len < sizeof(struct sockaddr_in))
>>  		goto out;
>>
>> +	/* BPF prog is run before any checks are done so that if the prog
>> +	 * changes context in a wrong way it will be caught.
>> +	 */
>> +	err = BPF_CGROUP_RUN_PROG_INET4_BIND(sk, uaddr);
>> +	if (err)
>> +		goto out;
>> +
>
> Should the hook not come at the very beginning?
>
>         /* If the socket has its own bind function then use it. (RAW) */
>         if (sk->sk_prot->bind) {
>                 err = sk->sk_prot->bind(sk, uaddr, addr_len);
>                 goto out;
>         }
>         err = -EINVAL;
>         if (addr_len < sizeof(struct sockaddr_in))
>                 goto out;
>
>         /* BPF prog is run before any checks are done so that if the prog
>          * changes context in a wrong way it will be caught.
>          */
>         err = BPF_CGROUP_RUN_PROG_INET4_BIND(sk, uaddr);
>         if (err)
>                 goto out;
>
> E.g. when you have v4/v6 ping or raw sockets used from language runtimes
> or apps, then they provide their own bind handler here in kernel, thus any
> bind rewrite won't be caught for them. Shouldn't this be covered as well
> and the BPF_CGROUP_RUN_PROG_INET4_BIND() come first?

the reason for hook to be called after
'if (addr_len < sizeof(struct sockaddr_in))' check is that
'struct bpf_sock_addr' rewrite assumes either sockaddr_in
or sockaddr_in6 when accessing fields.

For example, raw_bind(s) have a variety of sockaddr_* types that
we cannot recognize from bpf side without introducing special
ctx rewriter for each possible protocol and different bpf ctx for each.

That's why the hooks are called INET4_BIND and INET6_BIND and
later in __cgroup_bpf_run_filter_sock_addr() we do:
         if (sk->sk_family != AF_INET && sk->sk_family != AF_INET6)
                 return 0;

I don't think it's possible to have one generic bind hook
for all sk_proto. What do we pass into bpf prog as context?
They all have different sockaddr*.
Consider sockaddr_sco vs sockaddr_can, etc.
In the future this feature can be extend with per-protocol bind hooks
(if really necessary), but the hooks probably will be inside specific
raw_bind() functions instead of here.

The crazy alternative approach would be to pass blob of bytes into
bpf prog as ctx and let program parse it differently depending
on protocol, but then we'd need to make 'struct bpf_sock_addr'
variable length or size it up to the largest possible sockaddr_*.
Sanitizing fields becomes complex and so on. That won't be clean.