On Fri, May 25, 2018 at 10:37 AM, John Fastabend <john.fastab...@gmail.com> wrote: > syzbot reported two related splats, a use after free and null > pointer dereference, when a TCP socket is closed while the map is > also being removed. > > The psock keeps a reference to all map slots that have a reference > to the sock so that when the sock is closed we can clean up any > outstanding sock{map|hash} entries. This avoids pinning a sock > forever if the map owner fails to do proper cleanup. However, the > result is we have two paths that can free an entry in the map. Even > the comment in the sock{map|hash} tear down function, sock_hash_free() > notes this: > > At this point no update, lookup or delete operations can happen. > However, be aware we can still get a socket state event updates, > and data ready callbacks that reference the psock from sk_user_data. > > Both removal paths omitted taking the hash bucket lock resulting > in the case where we have two references that are in the process > of being free'd. > > Reported-by: syzbot+a761b81c211794fa1...@syzkaller.appspotmail.com > Signed-off-by: John Fastabend <john.fastab...@gmail.com>
Acked-by: Song Liu <songliubrav...@fb.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 52a91d8..b508141f 100644 > --- a/kernel/bpf/sockmap.c > +++ b/kernel/bpf/sockmap.c > @@ -225,6 +225,16 @@ static void free_htab_elem(struct bpf_htab *htab, struct > htab_elem *l) > kfree_rcu(l, rcu); > } > > +static inline struct bucket *__select_bucket(struct bpf_htab *htab, u32 hash) > +{ > + return &htab->buckets[hash & (htab->n_buckets - 1)]; > +} > + > +static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 > hash) > +{ > + return &__select_bucket(htab, hash)->head; > +} > + > static void bpf_tcp_close(struct sock *sk, long timeout) > { > void (*close_fun)(struct sock *sk, long timeout); > @@ -268,9 +278,15 @@ static void bpf_tcp_close(struct sock *sk, long timeout) > smap_release_sock(psock, sk); > } > } else { > + u32 hash = e->hash_link->hash; > + struct bucket *b; > + > + b = __select_bucket(e->htab, hash); > + raw_spin_lock_bh(&b->lock); > hlist_del_rcu(&e->hash_link->hash_node); > smap_release_sock(psock, e->hash_link->sk); > free_htab_elem(e->htab, e->hash_link); > + raw_spin_unlock_bh(&b->lock); > } > } > write_unlock_bh(&sk->sk_callback_lock); > @@ -2043,16 +2059,6 @@ static struct bpf_map *sock_hash_alloc(union bpf_attr > *attr) > return ERR_PTR(err); > } > > -static inline struct bucket *__select_bucket(struct bpf_htab *htab, u32 hash) > -{ > - return &htab->buckets[hash & (htab->n_buckets - 1)]; > -} > - > -static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 > hash) > -{ > - return &__select_bucket(htab, hash)->head; > -} > - > static void sock_hash_free(struct bpf_map *map) > { > struct bpf_htab *htab = container_of(map, struct bpf_htab, map); > @@ -2069,10 +2075,12 @@ static void sock_hash_free(struct bpf_map *map) > */ > rcu_read_lock(); > for (i = 0; i < htab->n_buckets; i++) { > - struct hlist_head *head = select_bucket(htab, i); > + struct bucket *b = __select_bucket(htab, i); > + struct hlist_head *head = &b->head; > struct hlist_node *n; > struct htab_elem *l; > > + raw_spin_lock_bh(&b->lock); > hlist_for_each_entry_safe(l, n, head, hash_node) { > struct sock *sock = l->sk; > struct smap_psock *psock; > @@ -2090,8 +2098,9 @@ static void sock_hash_free(struct bpf_map *map) > smap_release_sock(psock, sock); > } > write_unlock_bh(&sock->sk_callback_lock); > - kfree(l); > + free_htab_elem(htab, l); > } > + raw_spin_unlock_bh(&b->lock); > } > rcu_read_unlock(); > bpf_map_area_free(htab->buckets); >