netdev - Re: [bpf-next PATCH] bpf: sockmap, fix crash when ipv6 sock is added

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: <4618826b-3ab7-4fe0-0c3b-c6643defc921@iogearbox.net>
Date:   Tue, 5 Jun 2018 01:20:50 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>, edumazet@...gle.com,
        ast@...nel.org
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf-next PATCH] bpf: sockmap, fix crash when ipv6 sock is added

On 06/05/2018 01:08 AM, John Fastabend wrote:
> On 06/04/2018 12:59 PM, Daniel Borkmann wrote:
>> On 06/04/2018 05:21 PM, John Fastabend wrote:
>>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>>> of tcpv6_prot.
>>>
>>> Previously we overwrote the sk->prot field with tcp_prot even in the
>>> AF_INET6 case. This patch ensures the correct tcp_prot and tcpv6_prot
>>> are used. Further, only allow ESTABLISHED connections to join the
>>> map per note in TLS ULP,
>>>
>>>    /* The TLS ulp is currently supported only for TCP sockets
>>>     * in ESTABLISHED state.
>>>     * Supporting sockets in LISTEN state will require us
>>>     * to modify the accept implementation to clone rather then
>>>     * share the ulp context.
>>>     */
>>>
>>> Also tested with 'netserver -6' and 'netperf -H [IPv6]' as well as
>>> 'netperf -H [IPv4]'. The ESTABLISHED check resolves the previously
>>> crashing case here.
>>>
>>> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
>>> Reported-by: syzbot+5c063698bdbfac19f363@...kaller.appspotmail.com
>>> Signed-off-by: John Fastabend <john.fastabend@...il.com>
>>> Signed-off-by: Wei Wang <weiwan@...gle.com>
>>
>> Applied to bpf-next, thanks everyone!
> 
> Thanks Daniel, this has the unfortunate side-effect though of
> making it hard to add sockets transitioning from LISTEN into
> ESTABLISHED states to a sockmap. Before this patch we could add
> sockets from the sock_ops event BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB
> to a sock{map|hash}. However, this is before a socket is in established
> state so risked crashing and wasn't valid at all per this thread. So
> I believe its correct to block this action, seeing it will crash a
> system in many (most!) cases.
> 
> That said we still would like to support pushing sockets into a
> sock{map|hash} in this case. I thought about adding a new hook but
> we already have a few sock op hooks in the TCP stack so its too bad we
> don't have one that fires after the ESTABLISHED state has transitioned.
> Right now I'm looking into seeing if the following would have any
> issues,
> 
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2206,9 +2206,6 @@ void tcp_set_state(struct sock *sk, int state)
>         BUILD_BUG_ON((int)BPF_TCP_NEW_SYN_RECV != (int)TCP_NEW_SYN_RECV);
>         BUILD_BUG_ON((int)BPF_TCP_MAX_STATES != (int)TCP_MAX_STATES);
>  
> -       if (BPF_SOCK_OPS_TEST_FLAG(tcp_sk(sk), BPF_SOCK_OPS_STATE_CB_FLAG))
> -               tcp_call_bpf_2arg(sk, BPF_SOCK_OPS_STATE_CB, oldstate, state);
> -
>         switch (state) {
>         case TCP_ESTABLISHED:
>                 if (oldstate != TCP_ESTABLISHED)
> @@ -2234,6 +2231,9 @@ void tcp_set_state(struct sock *sk, int state)
>          */
>         inet_sk_state_store(sk, state);
>  
> +       if (BPF_SOCK_OPS_TEST_FLAG(tcp_sk(sk), BPF_SOCK_OPS_STATE_CB_FLAG))
> +               tcp_call_bpf_2arg(sk, BPF_SOCK_OPS_STATE_CB, oldstate, state);
> +
>  #ifdef STATE_TRACE
>         SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n", sk, statename[oldstate], statename[state]);
>  #endif
> 
> This would change the call hook slightly, moving it to after the state
> change. However unless the unhash is some how visible from the bpf program
> I don't think it should impact existing BPF programs.

Hmm, the current fix also breaks compilation when IPv6 is compiled out, so I had
to take it out for now. :-( I think this needs similar workaround as in kTLS case
in tls_init(). Given this and your above seen side-effect, lets respin all with a
clean fix.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
head:   4e1f687e302835e45e2f296392f21cfeb5671303
commit: 4e1f687e302835e45e2f296392f21cfeb5671303 [3/3] bpf: sockmap, fix crash when ipv6 sock is added
config: i386-randconfig-a0-06041847 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        git checkout 4e1f687e302835e45e2f296392f21cfeb5671303
        # save the attached .config to linux build tree
        make ARCH=i386

All errors (new ones prefixed by >>):

   kernel/bpf/sockmap.o: In function `bpf_tcp_ulp_register':
>> kernel/bpf/sockmap.c:1127: undefined reference to `tcpv6_prot'
>> kernel/bpf/sockmap.c:1127: undefined reference to `tcpv6_prot'

vim +1127 kernel/bpf/sockmap.c

  1122	
  1123	static int bpf_tcp_ulp_register(void)
  1124	{
  1125		tcp_bpf_proto = tcp_prot;
  1126		tcp_bpf_proto.close = bpf_tcp_close;
> 1127		tcpv6_bpf_proto = tcpv6_prot;
  1128		tcpv6_bpf_proto.close = bpf_tcp_close;
  1129		/* Once BPF TX ULP is registered it is never unregistered. It
  1130		 * will be in the ULP list for the lifetime of the system. Doing
  1131		 * duplicate registers is not a problem.
  1132		 */
  1133		return tcp_register_ulp(&bpf_tcp_ulp_ops);
  1134	}
  1135	

Thanks,
Daniel