| From 5d5053842d54b0f8fbe5c650d1ec7447681b25ca Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Thu, 23 Dec 2021 13:04:30 -0500 |
| Subject: sctp: use call_rcu to free endpoint |
| |
| From: Xin Long <lucien.xin@gmail.com> |
| |
| [ Upstream commit 5ec7d18d1813a5bead0b495045606c93873aecbb ] |
| |
| This patch is to delay the endpoint free by calling call_rcu() to fix |
| another use-after-free issue in sctp_sock_dump(): |
| |
| BUG: KASAN: use-after-free in __lock_acquire+0x36d9/0x4c20 |
| Call Trace: |
| __lock_acquire+0x36d9/0x4c20 kernel/locking/lockdep.c:3218 |
| lock_acquire+0x1ed/0x520 kernel/locking/lockdep.c:3844 |
| __raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline] |
| _raw_spin_lock_bh+0x31/0x40 kernel/locking/spinlock.c:168 |
| spin_lock_bh include/linux/spinlock.h:334 [inline] |
| __lock_sock+0x203/0x350 net/core/sock.c:2253 |
| lock_sock_nested+0xfe/0x120 net/core/sock.c:2774 |
| lock_sock include/net/sock.h:1492 [inline] |
| sctp_sock_dump+0x122/0xb20 net/sctp/diag.c:324 |
| sctp_for_each_transport+0x2b5/0x370 net/sctp/socket.c:5091 |
| sctp_diag_dump+0x3ac/0x660 net/sctp/diag.c:527 |
| __inet_diag_dump+0xa8/0x140 net/ipv4/inet_diag.c:1049 |
| inet_diag_dump+0x9b/0x110 net/ipv4/inet_diag.c:1065 |
| netlink_dump+0x606/0x1080 net/netlink/af_netlink.c:2244 |
| __netlink_dump_start+0x59a/0x7c0 net/netlink/af_netlink.c:2352 |
| netlink_dump_start include/linux/netlink.h:216 [inline] |
| inet_diag_handler_cmd+0x2ce/0x3f0 net/ipv4/inet_diag.c:1170 |
| __sock_diag_cmd net/core/sock_diag.c:232 [inline] |
| sock_diag_rcv_msg+0x31d/0x410 net/core/sock_diag.c:263 |
| netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2477 |
| sock_diag_rcv+0x2a/0x40 net/core/sock_diag.c:274 |
| |
| This issue occurs when asoc is peeled off and the old sk is freed after |
| getting it by asoc->base.sk and before calling lock_sock(sk). |
| |
| To prevent the sk free, as a holder of the sk, ep should be alive when |
| calling lock_sock(). This patch uses call_rcu() and moves sock_put and |
| ep free into sctp_endpoint_destroy_rcu(), so that it's safe to try to |
| hold the ep under rcu_read_lock in sctp_transport_traverse_process(). |
| |
| If sctp_endpoint_hold() returns true, it means this ep is still alive |
| and we have held it and can continue to dump it; If it returns false, |
| it means this ep is dead and can be freed after rcu_read_unlock, and |
| we should skip it. |
| |
| In sctp_sock_dump(), after locking the sk, if this ep is different from |
| tsp->asoc->ep, it means during this dumping, this asoc was peeled off |
| before calling lock_sock(), and the sk should be skipped; If this ep is |
| the same with tsp->asoc->ep, it means no peeloff happens on this asoc, |
| and due to lock_sock, no peeloff will happen either until release_sock. |
| |
| Note that delaying endpoint free won't delay the port release, as the |
| port release happens in sctp_endpoint_destroy() before calling call_rcu(). |
| Also, freeing endpoint by call_rcu() makes it safe to access the sk by |
| asoc->base.sk in sctp_assocs_seq_show() and sctp_rcv(). |
| |
| Thanks Jones to bring this issue up. |
| |
| v1->v2: |
| - improve the changelog. |
| - add kfree(ep) into sctp_endpoint_destroy_rcu(), as Jakub noticed. |
| |
| Reported-by: syzbot+9276d76e83e3bcde6c99@syzkaller.appspotmail.com |
| Reported-by: Lee Jones <lee.jones@linaro.org> |
| Fixes: d25adbeb0cdb ("sctp: fix an use-after-free issue in sctp_sock_dump") |
| Signed-off-by: Xin Long <lucien.xin@gmail.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| include/net/sctp/sctp.h | 6 +++--- |
| include/net/sctp/structs.h | 3 ++- |
| net/sctp/diag.c | 12 ++++++------ |
| net/sctp/endpointola.c | 23 +++++++++++++++-------- |
| net/sctp/socket.c | 23 +++++++++++++++-------- |
| 5 files changed, 41 insertions(+), 26 deletions(-) |
| |
| diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h |
| index 2c6570e6fcfec..903b2ddde1b43 100644 |
| --- a/include/net/sctp/sctp.h |
| +++ b/include/net/sctp/sctp.h |
| @@ -118,6 +118,7 @@ extern struct percpu_counter sctp_sockets_allocated; |
| int sctp_asconf_mgmt(struct sctp_sock *, struct sctp_sockaddr_entry *); |
| struct sk_buff *sctp_skb_recv_datagram(struct sock *, int, int, int *); |
| |
| +typedef int (*sctp_callback_t)(struct sctp_endpoint *, struct sctp_transport *, void *); |
| void sctp_transport_walk_start(struct rhashtable_iter *iter); |
| void sctp_transport_walk_stop(struct rhashtable_iter *iter); |
| struct sctp_transport *sctp_transport_get_next(struct net *net, |
| @@ -128,9 +129,8 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *), |
| struct net *net, |
| const union sctp_addr *laddr, |
| const union sctp_addr *paddr, void *p); |
| -int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *), |
| - int (*cb_done)(struct sctp_transport *, void *), |
| - struct net *net, int *pos, void *p); |
| +int sctp_transport_traverse_process(sctp_callback_t cb, sctp_callback_t cb_done, |
| + struct net *net, int *pos, void *p); |
| int sctp_for_each_endpoint(int (*cb)(struct sctp_endpoint *, void *), void *p); |
| int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc, |
| struct sctp_info *info); |
| diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h |
| index 2882bc7a5b4b8..18f9924aa2507 100644 |
| --- a/include/net/sctp/structs.h |
| +++ b/include/net/sctp/structs.h |
| @@ -1348,6 +1348,7 @@ struct sctp_endpoint { |
| |
| u32 secid; |
| u32 peer_secid; |
| + struct rcu_head rcu; |
| }; |
| |
| /* Recover the outter endpoint structure. */ |
| @@ -1363,7 +1364,7 @@ static inline struct sctp_endpoint *sctp_ep(struct sctp_ep_common *base) |
| struct sctp_endpoint *sctp_endpoint_new(struct sock *, gfp_t); |
| void sctp_endpoint_free(struct sctp_endpoint *); |
| void sctp_endpoint_put(struct sctp_endpoint *); |
| -void sctp_endpoint_hold(struct sctp_endpoint *); |
| +int sctp_endpoint_hold(struct sctp_endpoint *ep); |
| void sctp_endpoint_add_asoc(struct sctp_endpoint *, struct sctp_association *); |
| struct sctp_association *sctp_endpoint_lookup_assoc( |
| const struct sctp_endpoint *ep, |
| diff --git a/net/sctp/diag.c b/net/sctp/diag.c |
| index 8767405de9fa4..0a9db0a7f4234 100644 |
| --- a/net/sctp/diag.c |
| +++ b/net/sctp/diag.c |
| @@ -307,9 +307,8 @@ out: |
| return err; |
| } |
| |
| -static int sctp_sock_dump(struct sctp_transport *tsp, void *p) |
| +static int sctp_sock_dump(struct sctp_endpoint *ep, struct sctp_transport *tsp, void *p) |
| { |
| - struct sctp_endpoint *ep = tsp->asoc->ep; |
| struct sctp_comm_param *commp = p; |
| struct sock *sk = ep->base.sk; |
| struct sk_buff *skb = commp->skb; |
| @@ -319,6 +318,8 @@ static int sctp_sock_dump(struct sctp_transport *tsp, void *p) |
| int err = 0; |
| |
| lock_sock(sk); |
| + if (ep != tsp->asoc->ep) |
| + goto release; |
| list_for_each_entry(assoc, &ep->asocs, asocs) { |
| if (cb->args[4] < cb->args[1]) |
| goto next; |
| @@ -361,9 +362,8 @@ release: |
| return err; |
| } |
| |
| -static int sctp_sock_filter(struct sctp_transport *tsp, void *p) |
| +static int sctp_sock_filter(struct sctp_endpoint *ep, struct sctp_transport *tsp, void *p) |
| { |
| - struct sctp_endpoint *ep = tsp->asoc->ep; |
| struct sctp_comm_param *commp = p; |
| struct sock *sk = ep->base.sk; |
| const struct inet_diag_req_v2 *r = commp->r; |
| @@ -521,8 +521,8 @@ skip: |
| if (!(idiag_states & ~(TCPF_LISTEN | TCPF_CLOSE))) |
| goto done; |
| |
| - sctp_for_each_transport(sctp_sock_filter, sctp_sock_dump, |
| - net, &pos, &commp); |
| + sctp_transport_traverse_process(sctp_sock_filter, sctp_sock_dump, |
| + net, &pos, &commp); |
| cb->args[2] = pos; |
| |
| done: |
| diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c |
| index 8640dedcf64f1..c4068451b9c76 100644 |
| --- a/net/sctp/endpointola.c |
| +++ b/net/sctp/endpointola.c |
| @@ -242,6 +242,18 @@ void sctp_endpoint_free(struct sctp_endpoint *ep) |
| } |
| |
| /* Final destructor for endpoint. */ |
| +static void sctp_endpoint_destroy_rcu(struct rcu_head *head) |
| +{ |
| + struct sctp_endpoint *ep = container_of(head, struct sctp_endpoint, rcu); |
| + struct sock *sk = ep->base.sk; |
| + |
| + sctp_sk(sk)->ep = NULL; |
| + sock_put(sk); |
| + |
| + kfree(ep); |
| + SCTP_DBG_OBJCNT_DEC(ep); |
| +} |
| + |
| static void sctp_endpoint_destroy(struct sctp_endpoint *ep) |
| { |
| struct sock *sk; |
| @@ -275,18 +287,13 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep) |
| if (sctp_sk(sk)->bind_hash) |
| sctp_put_port(sk); |
| |
| - sctp_sk(sk)->ep = NULL; |
| - /* Give up our hold on the sock */ |
| - sock_put(sk); |
| - |
| - kfree(ep); |
| - SCTP_DBG_OBJCNT_DEC(ep); |
| + call_rcu(&ep->rcu, sctp_endpoint_destroy_rcu); |
| } |
| |
| /* Hold a reference to an endpoint. */ |
| -void sctp_endpoint_hold(struct sctp_endpoint *ep) |
| +int sctp_endpoint_hold(struct sctp_endpoint *ep) |
| { |
| - refcount_inc(&ep->base.refcnt); |
| + return refcount_inc_not_zero(&ep->base.refcnt); |
| } |
| |
| /* Release a reference to an endpoint and clean up if there are |
| diff --git a/net/sctp/socket.c b/net/sctp/socket.c |
| index 1148f66917079..d429d59228041 100644 |
| --- a/net/sctp/socket.c |
| +++ b/net/sctp/socket.c |
| @@ -5056,11 +5056,12 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *), |
| } |
| EXPORT_SYMBOL_GPL(sctp_transport_lookup_process); |
| |
| -int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *), |
| - int (*cb_done)(struct sctp_transport *, void *), |
| - struct net *net, int *pos, void *p) { |
| +int sctp_transport_traverse_process(sctp_callback_t cb, sctp_callback_t cb_done, |
| + struct net *net, int *pos, void *p) |
| +{ |
| struct rhashtable_iter hti; |
| struct sctp_transport *tsp; |
| + struct sctp_endpoint *ep; |
| int ret; |
| |
| again: |
| @@ -5069,26 +5070,32 @@ again: |
| |
| tsp = sctp_transport_get_idx(net, &hti, *pos + 1); |
| for (; !IS_ERR_OR_NULL(tsp); tsp = sctp_transport_get_next(net, &hti)) { |
| - ret = cb(tsp, p); |
| - if (ret) |
| - break; |
| + ep = tsp->asoc->ep; |
| + if (sctp_endpoint_hold(ep)) { /* asoc can be peeled off */ |
| + ret = cb(ep, tsp, p); |
| + if (ret) |
| + break; |
| + sctp_endpoint_put(ep); |
| + } |
| (*pos)++; |
| sctp_transport_put(tsp); |
| } |
| sctp_transport_walk_stop(&hti); |
| |
| if (ret) { |
| - if (cb_done && !cb_done(tsp, p)) { |
| + if (cb_done && !cb_done(ep, tsp, p)) { |
| (*pos)++; |
| + sctp_endpoint_put(ep); |
| sctp_transport_put(tsp); |
| goto again; |
| } |
| + sctp_endpoint_put(ep); |
| sctp_transport_put(tsp); |
| } |
| |
| return ret; |
| } |
| -EXPORT_SYMBOL_GPL(sctp_for_each_transport); |
| +EXPORT_SYMBOL_GPL(sctp_transport_traverse_process); |
| |
| /* 7.2.1 Association Status (SCTP_STATUS) |
| |
| -- |
| 2.34.1 |
| |