| From 3d4ad3d6c35f19ea817bf1b6d98ba4a81e158d90 Mon Sep 17 00:00:00 2001 |
| From: Lorenz Bauer <lmb@cloudflare.com> |
| Date: Fri, 7 Feb 2020 10:37:12 +0000 |
| Subject: [PATCH] bpf, sockmap: Check update requirements after locking |
| |
| commit 85b8ac01a421791d66c3a458a7f83cfd173fe3fa upstream. |
| |
| It's currently possible to insert sockets in unexpected states into |
| a sockmap, due to a TOCTTOU when updating the map from a syscall. |
| sock_map_update_elem checks that sk->sk_state == TCP_ESTABLISHED, |
| locks the socket and then calls sock_map_update_common. At this |
| point, the socket may have transitioned into another state, and |
| the earlier assumptions don't hold anymore. Crucially, it's |
| conceivable (though very unlikely) that a socket has become unhashed. |
| This breaks the sockmap's assumption that it will get a callback |
| via sk->sk_prot->unhash. |
| |
| Fix this by checking the (fixed) sk_type and sk_protocol without the |
| lock, followed by a locked check of sk_state. |
| |
| Unfortunately it's not possible to push the check down into |
| sock_(map|hash)_update_common, since BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB |
| run before the socket has transitioned from TCP_SYN_RECV into |
| TCP_ESTABLISHED. |
| |
| Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") |
| Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> |
| Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> |
| Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> |
| Link: https://lore.kernel.org/bpf/20200207103713.28175-1-lmb@cloudflare.com |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/net/core/sock_map.c b/net/core/sock_map.c |
| index 53bf40ecc884..45c09b87092e 100644 |
| --- a/net/core/sock_map.c |
| +++ b/net/core/sock_map.c |
| @@ -422,14 +422,16 @@ 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; |
| } |
| |
| sock_map_sk_acquire(sk); |
| - ret = sock_map_update_common(map, idx, sk, flags); |
| + if (sk->sk_state != TCP_ESTABLISHED) |
| + ret = -EOPNOTSUPP; |
| + else |
| + ret = sock_map_update_common(map, idx, sk, flags); |
| sock_map_sk_release(sk); |
| out: |
| fput(sock->file); |
| @@ -745,14 +747,16 @@ static int sock_hash_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; |
| } |
| |
| sock_map_sk_acquire(sk); |
| - ret = sock_hash_update_common(map, key, sk, flags); |
| + if (sk->sk_state != TCP_ESTABLISHED) |
| + ret = -EOPNOTSUPP; |
| + else |
| + ret = sock_hash_update_common(map, key, sk, flags); |
| sock_map_sk_release(sk); |
| out: |
| fput(sock->file); |
| -- |
| 2.7.4 |
| |