mptcp_subflow_delegate() links the subflow context into a per-CPU napi list and takes a reference on the subflow socket. The context is a separate allocation, reached through icsk_ulp_data, and the list holds no reference to it. inet_csk_destroy_sock() runs sk_prot->destroy() regardless of the outstanding references. That path ends in tcp_cleanup_ulp(), which calls subflow_ulp_release(). subflow_ulp_release() frees the context with kfree_rcu() but does not remove it from the list. Once the grace period elapses and the context is actually freed, the next mptcp_napi_poll() dequeues that entry and mptcp_subflow_delegated_next() writes to the freed context with list_del_init(). That is the use-after-free. The freeing path cannot dequeue the entry itself: the list is per-CPU and only the CPU that queued the subflow may touch it, as tcp_release_cb_override() already notes. Delay the free instead. Add MPTCP_DELEGATE_DEAD. mptcp_subflow_free_ctx() sets it and frees the context only if the subflow is not scheduled, while mptcp_napi_poll() frees it if the dead bit is set when it clears the scheduled bit. Either mptcp_subflow_free_ctx() gets there first, sees the scheduled bit and leaves the free to mptcp_napi_poll(), or mptcp_napi_poll() gets there first, does not see the dead bit and mptcp_subflow_free_ctx() does the free. Also skip mptcp_subflow_process_delegated() once the dead bit is set. subflow_ulp_release() has already dropped the msk reference by then, so the pending actions must not run. Finally, move delegated_status out of struct_group(reset). mptcp_subflow_ctx_reset() clears that group on disconnect, and clearing the scheduled bit while the entry is still queued would let mptcp_subflow_free_ctx() free a context the napi list still points to. delegated_node is already outside the group. Fixes: b19bc2945b40 ("mptcp: implement delegated actions") Cc: stable@vger.kernel.org Signed-off-by: Hyunwoo Kim --- net/mptcp/protocol.c | 13 ++++++++++--- net/mptcp/protocol.h | 16 ++++++++++++++-- net/mptcp/subflow.c | 4 ++-- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 7c8180d8d5eff6..8aa276dfe88639 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -2641,7 +2641,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, */ if (!inet_csk(ssk)->icsk_ulp_ops) { WARN_ON_ONCE(!sock_flag(ssk, SOCK_DEAD)); - kfree_rcu(subflow, rcu); + mptcp_subflow_free_ctx(subflow); } else { /* otherwise tcp will dispose of the ssk and subflow ctx */ __tcp_close(ssk, 0); @@ -4617,10 +4617,13 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget) delegated = container_of(napi, struct mptcp_delegated_action, napi); while ((subflow = mptcp_subflow_delegated_next(delegated)) != NULL) { struct sock *ssk = mptcp_subflow_tcp_sock(subflow); + long status; bh_lock_sock_nested(ssk); if (!sock_owned_by_user(ssk)) { - mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0)); + status = xchg(&subflow->delegated_status, 0); + if (!(status & BIT(MPTCP_DELEGATE_DEAD))) + mptcp_subflow_process_delegated(ssk, status); } else { /* tcp_release_cb_override already processed * the action or will do at next release_sock(). @@ -4628,11 +4631,15 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget) * CPU that scheduled it. */ smp_wmb(); - clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status); + status = set_mask_bits(&subflow->delegated_status, + BIT(MPTCP_DELEGATE_SCHEDULED), 0); } bh_unlock_sock(ssk); sock_put(ssk); + if (status & BIT(MPTCP_DELEGATE_DEAD)) + kfree_rcu(subflow, rcu); + if (++work_done == budget) return budget; } diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 1b80f2d6ec5a23..df9d0bf127febd 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -538,8 +538,10 @@ DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions); #define MPTCP_DELEGATE_SEND 1 #define MPTCP_DELEGATE_ACK 2 #define MPTCP_DELEGATE_SNDBUF 3 +#define MPTCP_DELEGATE_DEAD 4 -#define MPTCP_DELEGATE_ACTIONS_MASK (~BIT(MPTCP_DELEGATE_SCHEDULED)) +#define MPTCP_DELEGATE_ACTIONS_MASK (~(BIT(MPTCP_DELEGATE_SCHEDULED) | \ + BIT(MPTCP_DELEGATE_DEAD))) /* MPTCP subflow context */ struct mptcp_subflow_context { struct list_head node;/* conn_list of subflows */ @@ -608,11 +610,11 @@ struct mptcp_subflow_context { u32 subflow_id; - long delegated_status; unsigned long fail_tout; ); + long delegated_status; struct list_head delegated_node; /* link into delegated_action, protected by local BH */ u32 setsockopt_seq; @@ -788,6 +790,16 @@ mptcp_subflow_delegated_next(struct mptcp_delegated_action *delegated) return ret; } +static inline void mptcp_subflow_free_ctx(struct mptcp_subflow_context *subflow) +{ + long old = set_mask_bits(&subflow->delegated_status, 0, + BIT(MPTCP_DELEGATE_DEAD)); + + /* a scheduled subflow is owned by mptcp_napi_poll(), which will free it */ + if (!(old & BIT(MPTCP_DELEGATE_SCHEDULED))) + kfree_rcu(subflow, rcu); +} + void __mptcp_inherit_memcg(struct sock *sk, struct sock *ssk, gfp_t gfp); void __mptcp_inherit_cgrp_data(struct sock *sk, struct sock *ssk); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index e1f20ff8fdb424..b118594ab1b9a8 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -786,7 +786,7 @@ void mptcp_subflow_drop_ctx(struct sock *ssk) sock_put(ctx->conn); } - kfree_rcu(ctx, rcu); + mptcp_subflow_free_ctx(ctx); } void __mptcp_subflow_fully_established(struct mptcp_sock *msk, @@ -2024,7 +2024,7 @@ static void subflow_ulp_release(struct sock *ssk) mptcp_subflow_ops_undo_override(ssk); if (release) - kfree_rcu(ctx, rcu); + mptcp_subflow_free_ctx(ctx); } static void subflow_ulp_clone(const struct request_sock *req, -- 2.43.0