From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexei Starovoitov Subject: Re: [PATCH v2 bpf-next 3/9] bpf: Hooks for sys_bind Date: Thu, 29 Mar 2018 17:01:17 -0700 Message-ID: References: <20180328034140.291484-1-ast@kernel.org> <20180328034140.291484-4-ast@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Daniel Borkmann , Alexei Starovoitov , Andrey Ignatov Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:42462 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751191AbeC3ACj (ORCPT ); Thu, 29 Mar 2018 20:02:39 -0400 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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.