| From 85b8ac01a421791d66c3a458a7f83cfd173fe3fa Mon Sep 17 00:00:00 2001 |
| From: Lorenz Bauer <lmb@cloudflare.com> |
| Date: Fri, 7 Feb 2020 10:37:12 +0000 |
| Subject: bpf, sockmap: Check update requirements after locking |
| |
| From: Lorenz Bauer <lmb@cloudflare.com> |
| |
| 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| net/core/sock_map.c | 16 ++++++++++------ |
| 1 file changed, 10 insertions(+), 6 deletions(-) |
| |
| --- a/net/core/sock_map.c |
| +++ b/net/core/sock_map.c |
| @@ -417,14 +417,16 @@ static int sock_map_update_elem(struct b |
| 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); |
| @@ -740,14 +742,16 @@ static int sock_hash_update_elem(struct |
| 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); |