netdev - Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case

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:   Tue, 3 Jul 2018 16:40:41 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>, ast@...nel.org
Cc:     netdev@...r.kernel.org, kafai@...com
Subject: Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in
 multi-map case

On 06/30/2018 03:51 PM, John Fastabend wrote:
> The current code, in the error path of sock_hash_ctx_update_elem,
> checks if the sock has a psock in the user data and if so decrements
> the reference count of the psock. However, if the error happens early
> in the error path we may have never incremented the psock reference
> count and if the psock exists because the sock is in another map then
> we may inadvertently decrement the reference count.
> 
> Fix this by making the error path only call smap_release_sock if the
> error happens after the increment.
> 
> Reported-by: syzbot+d464d2c20c717ef5a6a8@...kaller.appspotmail.com
> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---
>  0 files changed
> 
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index 4fc2cb1..63fb047 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -1896,7 +1896,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
>  		e = kzalloc(sizeof(*e), GFP_ATOMIC | __GFP_NOWARN);
>  		if (!e) {
>  			err = -ENOMEM;
> -			goto out_progs;
> +			goto out_free;
>  		}
>  	}
>  
> @@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>  	if (err)
>  		goto err;
>  
> -	/* bpf_map_update_elem() can be called in_irq() */
> +	psock = smap_psock_sk(sock);
> +	if (unlikely(!psock)) {
> +		err = -EINVAL;
> +		goto err;
> +	}

Is an error even possible at this point? If __sock_map_ctx_update_elem() succeeds,
we either allocated and linked a new psock to the sock or we inc'ed the existing
one's refcount. From my reading it seems we should always succeed the subsequent
smap_psock_sk(). If we would have failed here in between it would mean we'd have
a refcount imbalance somewhere?

> +
>  	raw_spin_lock_bh(&b->lock);
>  	l_old = lookup_elem_raw(head, hash, key, key_size);
>  	if (l_old && map_flags == BPF_NOEXIST) {
> @@ -2342,12 +2347,6 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>  		goto bucket_err;
>  	}
>  
> -	psock = smap_psock_sk(sock);
> -	if (unlikely(!psock)) {
> -		err = -EINVAL;
> -		goto bucket_err;
> -	}
> -
>  	rcu_assign_pointer(e->hash_link, l_new);
>  	rcu_assign_pointer(e->htab,
>  			   container_of(map, struct bpf_htab, map));
> @@ -2370,12 +2369,10 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>  	raw_spin_unlock_bh(&b->lock);
>  	return 0;
>  bucket_err:
> +	smap_release_sock(psock, sock);
>  	raw_spin_unlock_bh(&b->lock);
>  err:
>  	kfree(e);
> -	psock = smap_psock_sk(sock);
> -	if (psock)
> -		smap_release_sock(psock, sock);
>  	return err;
>  }
>  
> 

Thanks,
Daniel