From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-by2nam01on0099.outbound.protection.outlook.com ([104.47.34.99]:38331 "EHLO NAM01-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727177AbeH3WF7 (ORCPT ); Thu, 30 Aug 2018 18:05:59 -0400 From: Sasha Levin To: "stable@vger.kernel.org" CC: Daniel Borkmann , Alexei Starovoitov , Sasha Levin Subject: [PATCH AUTOSEL 4.18 032/113] bpf, sockmap: fix sock_map_ctx_update_elem race with exist/noexist Date: Thu, 30 Aug 2018 18:02:36 +0000 Message-ID: <20180830180050.35735-32-alexander.levin@microsoft.com> References: <20180830180050.35735-1-alexander.levin@microsoft.com> In-Reply-To: <20180830180050.35735-1-alexander.levin@microsoft.com> Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Sender: stable-owner@vger.kernel.org List-ID: From: Daniel Borkmann [ Upstream commit 585f5a6252ee43ec8feeee07387e3fcc7e8bb292 ] The current code in sock_map_ctx_update_elem() allows for BPF_EXIST and BPF_NOEXIST map update flags. While on array-like maps this approach is rather uncommon, e.g. bpf_fd_array_map_update_elem() and others enforce map update flags to be BPF_ANY such that xchg() can be used directly, the current implementation in sock map does not guarantee that such operation with BPF_EXIST / BPF_NOEXIST is atomic. The initial test does a READ_ONCE(stab->sock_map[i]) to fetch the socket from the slot which is then tested for NULL / non-NULL. However later after __sock_map_ctx_update_elem(), the actual update is done through osock =3D xchg(&stab->sock_map[i], sock). Problem is that in the meantime a different CPU could have updated / deleted a socket on that specific slot and thus flag contraints won't hold anymore. I've been thinking whether best would be to just break UAPI and do an enforcement of BPF_ANY to check if someone actually complains, however trouble is that already in BPF kselftest we use BPF_NOEXIST for the map update, and therefore it might have been copied into applications already. The fix to keep the current behavior intact would be to add a map lock similar to the sock hash bucket lock only for covering the whole map. Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support") Signed-off-by: Daniel Borkmann Acked-by: John Fastabend Acked-by: Song Liu Signed-off-by: Alexei Starovoitov Signed-off-by: Sasha Levin --- kernel/bpf/sockmap.c | 106 +++++++++++++++++++++++-------------------- 1 file changed, 57 insertions(+), 49 deletions(-) diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c index 694da74d7df0..871ce45443ed 100644 --- a/kernel/bpf/sockmap.c +++ b/kernel/bpf/sockmap.c @@ -58,6 +58,7 @@ struct bpf_stab { struct bpf_map map; struct sock **sock_map; struct bpf_sock_progs progs; + raw_spinlock_t lock; }; =20 struct bucket { @@ -89,9 +90,9 @@ enum smap_psock_state { =20 struct smap_psock_map_entry { struct list_head list; + struct bpf_map *map; struct sock **entry; struct htab_elem __rcu *hash_link; - struct bpf_htab __rcu *htab; }; =20 struct smap_psock { @@ -343,13 +344,18 @@ static void bpf_tcp_close(struct sock *sk, long timeo= ut) e =3D psock_map_pop(sk, psock); while (e) { if (e->entry) { - osk =3D cmpxchg(e->entry, sk, NULL); + struct bpf_stab *stab =3D container_of(e->map, struct bpf_stab, map); + + raw_spin_lock_bh(&stab->lock); + osk =3D *e->entry; if (osk =3D=3D sk) { + *e->entry =3D NULL; smap_release_sock(psock, sk); } + raw_spin_unlock_bh(&stab->lock); } else { struct htab_elem *link =3D rcu_dereference(e->hash_link); - struct bpf_htab *htab =3D rcu_dereference(e->htab); + struct bpf_htab *htab =3D container_of(e->map, struct bpf_htab, map); struct hlist_head *head; struct htab_elem *l; struct bucket *b; @@ -1644,6 +1650,7 @@ static struct bpf_map *sock_map_alloc(union bpf_attr = *attr) return ERR_PTR(-ENOMEM); =20 bpf_map_init_from_attr(&stab->map, attr); + raw_spin_lock_init(&stab->lock); =20 /* make sure page count doesn't overflow */ cost =3D (u64) stab->map.max_entries * sizeof(struct sock *); @@ -1714,14 +1721,15 @@ static void sock_map_free(struct bpf_map *map) * and a grace period expire to ensure psock is really safe to remove. */ rcu_read_lock(); + raw_spin_lock_bh(&stab->lock); for (i =3D 0; i < stab->map.max_entries; i++) { struct smap_psock *psock; struct sock *sock; =20 - sock =3D xchg(&stab->sock_map[i], NULL); + sock =3D stab->sock_map[i]; if (!sock) continue; - + stab->sock_map[i] =3D NULL; psock =3D smap_psock_sk(sock); /* This check handles a racing sock event that can get the * sk_callback_lock before this case but after xchg happens @@ -1733,6 +1741,7 @@ static void sock_map_free(struct bpf_map *map) smap_release_sock(psock, sock); } } + raw_spin_unlock_bh(&stab->lock); rcu_read_unlock(); =20 sock_map_remove_complete(stab); @@ -1776,14 +1785,16 @@ static int sock_map_delete_elem(struct bpf_map *map= , void *key) if (k >=3D map->max_entries) return -EINVAL; =20 - sock =3D xchg(&stab->sock_map[k], NULL); + raw_spin_lock_bh(&stab->lock); + sock =3D stab->sock_map[k]; + stab->sock_map[k] =3D NULL; + raw_spin_unlock_bh(&stab->lock); if (!sock) return -EINVAL; =20 psock =3D smap_psock_sk(sock); if (!psock) - goto out; - + return 0; if (psock->bpf_parse) { write_lock_bh(&sock->sk_callback_lock); smap_stop_sock(psock, sock); @@ -1791,7 +1802,6 @@ static int sock_map_delete_elem(struct bpf_map *map, = void *key) } smap_list_map_remove(psock, &stab->sock_map[k]); smap_release_sock(psock, sock); -out: return 0; } =20 @@ -1827,11 +1837,9 @@ static int sock_map_delete_elem(struct bpf_map *map,= void *key) static int __sock_map_ctx_update_elem(struct bpf_map *map, struct bpf_sock_progs *progs, struct sock *sock, - struct sock **map_link, void *key) { struct bpf_prog *verdict, *parse, *tx_msg; - struct smap_psock_map_entry *e =3D NULL; struct smap_psock *psock; bool new =3D false; int err =3D 0; @@ -1904,14 +1912,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map= *map, new =3D true; } =20 - if (map_link) { - e =3D kzalloc(sizeof(*e), GFP_ATOMIC | __GFP_NOWARN); - if (!e) { - err =3D -ENOMEM; - goto out_free; - } - } - /* 3. At this point we have a reference to a valid psock that is * running. Attach any BPF programs needed. */ @@ -1933,17 +1933,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map= *map, write_unlock_bh(&sock->sk_callback_lock); } =20 - /* 4. Place psock in sockmap for use and stop any programs on - * the old sock assuming its not the same sock we are replacing - * it with. Because we can only have a single set of programs if - * old_sock has a strp we can stop it. - */ - if (map_link) { - e->entry =3D map_link; - spin_lock_bh(&psock->maps_lock); - list_add_tail(&e->list, &psock->maps); - spin_unlock_bh(&psock->maps_lock); - } return err; out_free: smap_release_sock(psock, sock); @@ -1954,7 +1943,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map = *map, } if (tx_msg) bpf_prog_put(tx_msg); - kfree(e); return err; } =20 @@ -1964,36 +1952,57 @@ static int sock_map_ctx_update_elem(struct bpf_sock= _ops_kern *skops, { struct bpf_stab *stab =3D container_of(map, struct bpf_stab, map); struct bpf_sock_progs *progs =3D &stab->progs; - struct sock *osock, *sock; + struct sock *osock, *sock =3D skops->sk; + struct smap_psock_map_entry *e; + struct smap_psock *psock; u32 i =3D *(u32 *)key; int err; =20 if (unlikely(flags > BPF_EXIST)) return -EINVAL; - if (unlikely(i >=3D stab->map.max_entries)) return -E2BIG; =20 - sock =3D READ_ONCE(stab->sock_map[i]); - if (flags =3D=3D BPF_EXIST && !sock) - return -ENOENT; - else if (flags =3D=3D BPF_NOEXIST && sock) - return -EEXIST; + e =3D kzalloc(sizeof(*e), GFP_ATOMIC | __GFP_NOWARN); + if (!e) + return -ENOMEM; =20 - sock =3D skops->sk; - err =3D __sock_map_ctx_update_elem(map, progs, sock, &stab->sock_map[i], - key); + err =3D __sock_map_ctx_update_elem(map, progs, sock, key); if (err) goto out; =20 - osock =3D xchg(&stab->sock_map[i], sock); - if (osock) { - struct smap_psock *opsock =3D smap_psock_sk(osock); + /* psock guaranteed to be present. */ + psock =3D smap_psock_sk(sock); + raw_spin_lock_bh(&stab->lock); + osock =3D stab->sock_map[i]; + if (osock && flags =3D=3D BPF_NOEXIST) { + err =3D -EEXIST; + goto out_unlock; + } + if (!osock && flags =3D=3D BPF_EXIST) { + err =3D -ENOENT; + goto out_unlock; + } + + e->entry =3D &stab->sock_map[i]; + e->map =3D map; + spin_lock_bh(&psock->maps_lock); + list_add_tail(&e->list, &psock->maps); + spin_unlock_bh(&psock->maps_lock); =20 - smap_list_map_remove(opsock, &stab->sock_map[i]); - smap_release_sock(opsock, osock); + stab->sock_map[i] =3D sock; + if (osock) { + psock =3D smap_psock_sk(osock); + smap_list_map_remove(psock, &stab->sock_map[i]); + smap_release_sock(psock, osock); } + raw_spin_unlock_bh(&stab->lock); + return 0; +out_unlock: + smap_release_sock(psock, sock); + raw_spin_unlock_bh(&stab->lock); out: + kfree(e); return err; } =20 @@ -2356,7 +2365,7 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_= ops_kern *skops, b =3D __select_bucket(htab, hash); head =3D &b->head; =20 - err =3D __sock_map_ctx_update_elem(map, progs, sock, NULL, key); + err =3D __sock_map_ctx_update_elem(map, progs, sock, key); if (err) goto err; =20 @@ -2382,8 +2391,7 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_= ops_kern *skops, } =20 rcu_assign_pointer(e->hash_link, l_new); - rcu_assign_pointer(e->htab, - container_of(map, struct bpf_htab, map)); + e->map =3D map; spin_lock_bh(&psock->maps_lock); list_add_tail(&e->list, &psock->maps); spin_unlock_bh(&psock->maps_lock); --=20 2.17.1