netdev - RE: [PATCH bpf-next v2 05/11] bpf, sockmap: Allow inserting listening TCP sockets into sockmap

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]

Date:   Sat, 11 Jan 2020 15:59:23 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>, bpf@...r.kernel.org
Cc:     netdev@...r.kernel.org, kernel-team@...udflare.com,
        Eric Dumazet <edumazet@...gle.com>,
        John Fastabend <john.fastabend@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Martin KaFai Lau <kafai@...com>
Subject: RE: [PATCH bpf-next v2 05/11] bpf, sockmap: Allow inserting listening
 TCP sockets into sockmap

Jakub Sitnicki wrote:
> In order for sockmap type to become a generic collection for storing TCP
> sockets we need to loosen the checks during map update, while tightening
> the checks in redirect helpers.
> 
> Currently sockmap requires the TCP socket to be in established state (or
> transitioning out of SYN_RECV into established state when done from BPF),
> which prevents inserting listening sockets.
> 
> Change the update pre-checks so that the socket can also be in listening
> state. If the state is not white-listed, return -EINVAL to be consistent
> with REUSEPORT_SOCKARRY map type.
> 
> Since it doesn't make sense to redirect with sockmap to listening sockets,
> add appropriate socket state checks to BPF redirect helpers too.
> 
> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> ---
>  net/core/sock_map.c                     | 46 ++++++++++++++++++++-----
>  tools/testing/selftests/bpf/test_maps.c |  6 +---
>  2 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index eb114ee419b6..99daea502508 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -396,6 +396,23 @@ static bool sock_map_sk_is_suitable(const struct sock *sk)
>  	       sk->sk_protocol == IPPROTO_TCP;
>  }
>  
> +/* Is sock in a state that allows inserting into the map?
> + * SYN_RECV is needed for updates on BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB.
> + */
> +static bool sock_map_update_okay(const struct sock *sk)
> +{
> +	return (1 << sk->sk_state) & (TCPF_ESTABLISHED |
> +				      TCPF_SYN_RECV |
> +				      TCPF_LISTEN);
> +}
> +
> +/* Is sock in a state that allows redirecting into it? */
> +static bool sock_map_redirect_okay(const struct sock *sk)
> +{
> +	return (1 << sk->sk_state) & (TCPF_ESTABLISHED |
> +				      TCPF_SYN_RECV);
> +}
> +
>  static int sock_map_update_elem(struct bpf_map *map, void *key,
>  				void *value, u64 flags)
>  {
> @@ -413,11 +430,14 @@ static int sock_map_update_elem(struct bpf_map *map, void *key,
>  		ret = -EINVAL;
>  		goto out;
>  	}
> -	if (!sock_map_sk_is_suitable(sk) ||
> -	    sk->sk_state != TCP_ESTABLISHED) {
> +	if (!sock_map_sk_is_suitable(sk)) {
>  		ret = -EOPNOTSUPP;
>  		goto out;
>  	}
> +	if (!sock_map_update_okay(sk)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

I nit but seeing we need a v3 anyways. How about consolidating
this state checks into sock_map_sk_is_suitable() so we don't have
multiple if branches or this '|| TCP_ESTABLISHED' like we do now.

>  
>  	sock_map_sk_acquire(sk);
>  	ret = sock_map_update_common(map, idx, sk, flags);
> @@ -433,6 +453,7 @@ BPF_CALL_4(bpf_sock_map_update, struct bpf_sock_ops_kern *, sops,
>  	WARN_ON_ONCE(!rcu_read_lock_held());
>  
>  	if (likely(sock_map_sk_is_suitable(sops->sk) &&
> +		   sock_map_update_okay(sops->sk) &&
>  		   sock_map_op_okay(sops)))
>  		return sock_map_update_common(map, *(u32 *)key, sops->sk,
>  					      flags);
> @@ -454,13 +475,17 @@ BPF_CALL_4(bpf_sk_redirect_map, struct sk_buff *, skb,
>  	   struct bpf_map *, map, u32, key, u64, flags)
>  {
>  	struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
> +	struct sock *sk;
>  
>  	if (unlikely(flags & ~(BPF_F_INGRESS)))
>  		return SK_DROP;
> -	tcb->bpf.flags = flags;
> -	tcb->bpf.sk_redir = __sock_map_lookup_elem(map, key);
> -	if (!tcb->bpf.sk_redir)
> +
> +	sk = __sock_map_lookup_elem(map, key);
> +	if (!sk || !sock_map_redirect_okay(sk))
>  		return SK_DROP;

unlikely(!sock_map_redirect_okay)? Or perhaps unlikely the entire case,
if (unlikely(!sk || !sock_map_redirect_okay(sk)). I think users should
know if the sk is a valid sock or not and this is just catching the
error case. Any opinion?

Otherwise looks good.