netdev - Re: [bpf PATCH v3 2/4] bpf: sockmap, fix smap_list_map

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]

Message-ID: <20180623074515.tqzzvcyluf6ouso7@kafai-mbp.dhcp.thefacebook.com>
Date:   Sat, 23 Jun 2018 00:45:15 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     John Fastabend <john.fastabend@...il.com>
CC:     <ast@...nel.org>, <daniel@...earbox.net>, <netdev@...r.kernel.org>
Subject: Re: [bpf PATCH v3 2/4] bpf: sockmap, fix smap_list_map_remove when
 psock is in many maps

On Fri, Jun 22, 2018 at 08:21:39AM -0700, John Fastabend wrote:
> If a hashmap is free'd with open socks it removes the reference to
> the hash entry from the psock. If that is the last reference to the
> psock then it will also be free'd by the reference counting logic.
> However the current logic that removes the hash reference from the
> list of references is broken. In map_list_map_remove() we first check
s/map_list_map_remove/smap_list_remove/

> if the sockmap entry matches and then check if the hashmap entry
> matches. But, the sockmap entry sill always match because its NULL in
> this case which causes the first entry to be removed from the list.
> If this is always the "right" entry (because the user adds/removes
> entries in order) then everything is OK but otherwise a subsequent
> bpf_tcp_close() may reference a free'd object.
> 
> To fix this create two list handlers one for sockmap and one for
> sockhash.
> 
> Reported-by: syzbot+0ce137753c78f7b6acc1@...kaller.appspotmail.com
> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
One nit.  Other than that,

Acked-by: Martin KaFai Lau <kafai@...com>

> ---
>  kernel/bpf/sockmap.c |   33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index d7fd17a..69b26af 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -1602,17 +1602,26 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
>  	return ERR_PTR(err);
>  }
>  
> -static void smap_list_remove(struct smap_psock *psock,
> -			     struct sock **entry,
> -			     struct htab_elem *hash_link)
> +static void smap_list_map_remove(struct smap_psock *psock,
> +				 struct sock **entry)
>  {
>  	struct smap_psock_map_entry *e, *tmp;
>  
>  	list_for_each_entry_safe(e, tmp, &psock->maps, list) {
> -		if (e->entry == entry || e->hash_link == hash_link) {
> +		if (e->entry == entry)
> +			list_del(&e->list);
> +	}
> +}
Nit. Add an empty line.

> +static void smap_list_hash_remove(struct smap_psock *psock,
> +				  struct htab_elem *hash_link)
> +{
> +	struct smap_psock_map_entry *e, *tmp;
> +
> +	list_for_each_entry_safe(e, tmp, &psock->maps, list) {
> +		struct htab_elem *c = e->hash_link;
> +
> +		if (c == hash_link)
>  			list_del(&e->list);
> -			break;
> -		}
>  	}
>  }
>  
> @@ -1647,7 +1656,7 @@ static void sock_map_free(struct bpf_map *map)
>  		 * to be null and queued for garbage collection.
>  		 */
>  		if (likely(psock)) {
> -			smap_list_remove(psock, &stab->sock_map[i], NULL);
> +			smap_list_map_remove(psock, &stab->sock_map[i]);
>  			smap_release_sock(psock, sock);
>  		}
>  		write_unlock_bh(&sock->sk_callback_lock);
> @@ -1706,7 +1715,7 @@ static int sock_map_delete_elem(struct bpf_map *map, void *key)
>  
>  	if (psock->bpf_parse)
>  		smap_stop_sock(psock, sock);
> -	smap_list_remove(psock, &stab->sock_map[k], NULL);
> +	smap_list_map_remove(psock, &stab->sock_map[k]);
>  	smap_release_sock(psock, sock);
>  out:
>  	write_unlock_bh(&sock->sk_callback_lock);
> @@ -1908,7 +1917,7 @@ static int sock_map_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>  		struct smap_psock *opsock = smap_psock_sk(osock);
>  
>  		write_lock_bh(&osock->sk_callback_lock);
> -		smap_list_remove(opsock, &stab->sock_map[i], NULL);
> +		smap_list_map_remove(opsock, &stab->sock_map[i]);
>  		smap_release_sock(opsock, osock);
>  		write_unlock_bh(&osock->sk_callback_lock);
>  	}
> @@ -2124,7 +2133,7 @@ static void sock_hash_free(struct bpf_map *map)
>  			 * (psock) to be null and queued for garbage collection.
>  			 */
>  			if (likely(psock)) {
> -				smap_list_remove(psock, NULL, l);
> +				smap_list_hash_remove(psock, l);
>  				smap_release_sock(psock, sock);
>  			}
>  			write_unlock_bh(&sock->sk_callback_lock);
> @@ -2304,7 +2313,7 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>  		psock = smap_psock_sk(l_old->sk);
>  
>  		hlist_del_rcu(&l_old->hash_node);
> -		smap_list_remove(psock, NULL, l_old);
> +		smap_list_hash_remove(psock, l_old);
>  		smap_release_sock(psock, l_old->sk);
>  		free_htab_elem(htab, l_old);
>  	}
> @@ -2372,7 +2381,7 @@ static int sock_hash_delete_elem(struct bpf_map *map, void *key)
>  		 * to be null and queued for garbage collection.
>  		 */
>  		if (likely(psock)) {
> -			smap_list_remove(psock, NULL, l);
> +			smap_list_hash_remove(psock, l);
>  			smap_release_sock(psock, sock);
>  		}
>  		write_unlock_bh(&sock->sk_callback_lock);
>