From 8b9088f806e1ccd10c3d48b3b6d3d5d7855d92c5 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Sat, 13 Oct 2018 02:45:56 +0200 Subject: tcp, ulp: enforce sock_owned_by_me upon ulp init and cleanup Whenever the ULP data on the socket is mangled, enforce that the caller has the socket lock held as otherwise things may race with initialization and cleanup callbacks from ulp ops as both would mangle internal socket state. Joint work with John. Signed-off-by: Daniel Borkmann Signed-off-by: John Fastabend Signed-off-by: Alexei Starovoitov --- net/ipv4/tcp_ulp.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'net/ipv4/tcp_ulp.c') diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c index a5995bb2eaca..34e96353f115 100644 --- a/net/ipv4/tcp_ulp.c +++ b/net/ipv4/tcp_ulp.c @@ -123,6 +123,8 @@ void tcp_cleanup_ulp(struct sock *sk) { struct inet_connection_sock *icsk = inet_csk(sk); + sock_owned_by_me(sk); + if (!icsk->icsk_ulp_ops) return; @@ -140,6 +142,7 @@ int tcp_set_ulp(struct sock *sk, const char *name) const struct tcp_ulp_ops *ulp_ops; int err = 0; + sock_owned_by_me(sk); if (icsk->icsk_ulp_ops) return -EEXIST; @@ -168,6 +171,7 @@ int tcp_set_ulp_id(struct sock *sk, int ulp) const struct tcp_ulp_ops *ulp_ops; int err; + sock_owned_by_me(sk); if (icsk->icsk_ulp_ops) return -EEXIST; -- cgit v1.2.3 From 1243a51f6c05ecbb2c5c9e02fdcc1e7a06f76f26 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Sat, 13 Oct 2018 02:45:57 +0200 Subject: tcp, ulp: remove ulp bits from sockmap In order to prepare sockmap logic to be used in combination with kTLS we need to detangle it from ULP, and further split it in later commits into a generic API. Joint work with John. Signed-off-by: Daniel Borkmann Signed-off-by: John Fastabend Signed-off-by: Alexei Starovoitov --- include/net/tcp.h | 1 - kernel/bpf/sockmap.c | 39 ++++++++--------------------- net/ipv4/tcp_ulp.c | 71 ++++++++++------------------------------------------ 3 files changed, 23 insertions(+), 88 deletions(-) (limited to 'net/ipv4/tcp_ulp.c') diff --git a/include/net/tcp.h b/include/net/tcp.h index 0d2929223c70..8f5cef67fd35 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -2057,7 +2057,6 @@ struct tcp_ulp_ops { int tcp_register_ulp(struct tcp_ulp_ops *type); void tcp_unregister_ulp(struct tcp_ulp_ops *type); int tcp_set_ulp(struct sock *sk, const char *name); -int tcp_set_ulp_id(struct sock *sk, const int ulp); void tcp_get_available_ulp(char *buf, size_t len); void tcp_cleanup_ulp(struct sock *sk); diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c index 5d0677d808ae..de6f7a65c72b 100644 --- a/kernel/bpf/sockmap.c +++ b/kernel/bpf/sockmap.c @@ -182,6 +182,7 @@ enum { static struct proto *saved_tcpv6_prot __read_mostly; static DEFINE_SPINLOCK(tcpv6_prot_lock); static struct proto bpf_tcp_prots[SOCKMAP_NUM_PROTS][SOCKMAP_NUM_CONFIGS]; + static void build_protos(struct proto prot[SOCKMAP_NUM_CONFIGS], struct proto *base) { @@ -239,6 +240,13 @@ static int bpf_tcp_init(struct sock *sk) return 0; } +static int __init bpf_sock_init(void) +{ + build_protos(bpf_tcp_prots[SOCKMAP_IPV4], &tcp_prot); + return 0; +} +core_initcall(bpf_sock_init); + static void smap_release_sock(struct smap_psock *psock, struct sock *sock); static int free_start_sg(struct sock *sk, struct sk_msg_buff *md, bool charge); @@ -413,15 +421,6 @@ enum __sk_action { __SK_NONE, }; -static struct tcp_ulp_ops bpf_tcp_ulp_ops __read_mostly = { - .name = "bpf_tcp", - .uid = TCP_ULP_BPF, - .user_visible = false, - .owner = NULL, - .init = bpf_tcp_init, - .release = bpf_tcp_release, -}; - static int memcopy_from_iter(struct sock *sk, struct sk_msg_buff *md, struct iov_iter *from, int bytes) @@ -1236,16 +1235,6 @@ static void bpf_tcp_msg_add(struct smap_psock *psock, bpf_prog_put(orig_tx_msg); } -static int bpf_tcp_ulp_register(void) -{ - build_protos(bpf_tcp_prots[SOCKMAP_IPV4], &tcp_prot); - /* Once BPF TX ULP is registered it is never unregistered. It - * will be in the ULP list for the lifetime of the system. Doing - * duplicate registers is not a problem. - */ - return tcp_register_ulp(&bpf_tcp_ulp_ops); -} - static int smap_verdict_func(struct smap_psock *psock, struct sk_buff *skb) { struct bpf_prog *prog = READ_ONCE(psock->bpf_verdict); @@ -1491,7 +1480,7 @@ static void smap_release_sock(struct smap_psock *psock, struct sock *sock) { if (refcount_dec_and_test(&psock->refcnt)) { if (psock_is_smap_sk(sock)) - tcp_cleanup_ulp(sock); + bpf_tcp_release(sock); write_lock_bh(&sock->sk_callback_lock); smap_stop_sock(psock, sock); write_unlock_bh(&sock->sk_callback_lock); @@ -1666,10 +1655,6 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr) attr->value_size != 4 || attr->map_flags & ~SOCK_CREATE_FLAG_MASK) return ERR_PTR(-EINVAL); - err = bpf_tcp_ulp_register(); - if (err && err != -EEXIST) - return ERR_PTR(err); - stab = kzalloc(sizeof(*stab), GFP_USER); if (!stab) return ERR_PTR(-ENOMEM); @@ -1951,7 +1936,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map, if (tx_msg) bpf_tcp_msg_add(psock, sock, tx_msg); if (new) { - err = tcp_set_ulp_id(sock, TCP_ULP_BPF); + err = bpf_tcp_init(sock); if (err) goto out_free; } @@ -2187,10 +2172,6 @@ static struct bpf_map *sock_hash_alloc(union bpf_attr *attr) */ return ERR_PTR(-E2BIG); - err = bpf_tcp_ulp_register(); - if (err && err != -EEXIST) - return ERR_PTR(err); - htab = kzalloc(sizeof(*htab), GFP_USER); if (!htab) return ERR_PTR(-ENOMEM); diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c index 34e96353f115..a9162aa11af9 100644 --- a/net/ipv4/tcp_ulp.c +++ b/net/ipv4/tcp_ulp.c @@ -6,7 +6,7 @@ * */ -#include +#include #include #include #include @@ -29,18 +29,6 @@ static struct tcp_ulp_ops *tcp_ulp_find(const char *name) return NULL; } -static struct tcp_ulp_ops *tcp_ulp_find_id(const int ulp) -{ - struct tcp_ulp_ops *e; - - list_for_each_entry_rcu(e, &tcp_ulp_list, list) { - if (e->uid == ulp) - return e; - } - - return NULL; -} - static const struct tcp_ulp_ops *__tcp_ulp_find_autoload(const char *name) { const struct tcp_ulp_ops *ulp = NULL; @@ -63,18 +51,6 @@ static const struct tcp_ulp_ops *__tcp_ulp_find_autoload(const char *name) return ulp; } -static const struct tcp_ulp_ops *__tcp_ulp_lookup(const int uid) -{ - const struct tcp_ulp_ops *ulp; - - rcu_read_lock(); - ulp = tcp_ulp_find_id(uid); - if (!ulp || !try_module_get(ulp->owner)) - ulp = NULL; - rcu_read_unlock(); - return ulp; -} - /* Attach new upper layer protocol to the list * of available protocols. */ @@ -135,56 +111,35 @@ void tcp_cleanup_ulp(struct sock *sk) icsk->icsk_ulp_ops = NULL; } -/* Change upper layer protocol for socket */ -int tcp_set_ulp(struct sock *sk, const char *name) +static int __tcp_set_ulp(struct sock *sk, const struct tcp_ulp_ops *ulp_ops) { struct inet_connection_sock *icsk = inet_csk(sk); - const struct tcp_ulp_ops *ulp_ops; - int err = 0; + int err; - sock_owned_by_me(sk); + err = -EEXIST; if (icsk->icsk_ulp_ops) - return -EEXIST; - - ulp_ops = __tcp_ulp_find_autoload(name); - if (!ulp_ops) - return -ENOENT; - - if (!ulp_ops->user_visible) { - module_put(ulp_ops->owner); - return -ENOENT; - } + goto out_err; err = ulp_ops->init(sk); - if (err) { - module_put(ulp_ops->owner); - return err; - } + if (err) + goto out_err; icsk->icsk_ulp_ops = ulp_ops; return 0; +out_err: + module_put(ulp_ops->owner); + return err; } -int tcp_set_ulp_id(struct sock *sk, int ulp) +int tcp_set_ulp(struct sock *sk, const char *name) { - struct inet_connection_sock *icsk = inet_csk(sk); const struct tcp_ulp_ops *ulp_ops; - int err; sock_owned_by_me(sk); - if (icsk->icsk_ulp_ops) - return -EEXIST; - ulp_ops = __tcp_ulp_lookup(ulp); + ulp_ops = __tcp_ulp_find_autoload(name); if (!ulp_ops) return -ENOENT; - err = ulp_ops->init(sk); - if (err) { - module_put(ulp_ops->owner); - return err; - } - - icsk->icsk_ulp_ops = ulp_ops; - return 0; + return __tcp_set_ulp(sk, ulp_ops); } -- cgit v1.2.3 From aadd4355918fe6e9044a9042fa5968e0a0901681 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Tue, 16 Oct 2018 21:31:35 +0200 Subject: tcp, ulp: remove socket lock assertion on ULP cleanup Eric reported that syzkaller triggered a splat in tcp_cleanup_ulp() where assertion sock_owned_by_me() failed. This happened through inet_csk_prepare_forced_close() first releasing the socket lock, then calling into tcp_done(newsk) which is called after the inet_csk_prepare_forced_close() and therefore without the socket lock held. The sock_owned_by_me() assertion can generally be removed as the only place where tcp_cleanup_ulp() is called from now is out of inet_csk_destroy_sock() -> sk->sk_prot->destroy() where socket is in dead state and unreachable. Therefore, add a comment why the check is not needed instead. Fixes: 8b9088f806e1 ("tcp, ulp: enforce sock_owned_by_me upon ulp init and cleanup") Reported-by: Eric Dumazet Signed-off-by: Daniel Borkmann Signed-off-by: David S. Miller --- net/ipv4/tcp_ulp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'net/ipv4/tcp_ulp.c') diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c index a9162aa11af9..95df7f7f6328 100644 --- a/net/ipv4/tcp_ulp.c +++ b/net/ipv4/tcp_ulp.c @@ -99,8 +99,10 @@ void tcp_cleanup_ulp(struct sock *sk) { struct inet_connection_sock *icsk = inet_csk(sk); - sock_owned_by_me(sk); - + /* No sock_owned_by_me() check here as at the time the + * stack calls this function, the socket is dead and + * about to be destroyed. + */ if (!icsk->icsk_ulp_ops) return; -- cgit v1.2.3