Skip to content

Commit 2827f09

Browse files
Paolo Abenigregkh
Paolo Abeni
authored and
gregkh
committed
mptcp: use the workqueue to destroy unaccepted sockets
[ Upstream commit b6985b9 ] Backports notes: one simple conflict in net/mptcp/protocol.c with: commit a5ef058 ("net: introduce and use custom sockopt socket flag") Where the two commits add a new line for different actions in the same context in mptcp_stream_accept(). Christoph reported a UaF at token lookup time after having refactored the passive socket initialization part: BUG: KASAN: use-after-free in __token_bucket_busy+0x253/0x260 Read of size 4 at addr ffff88810698d5b0 by task syz-executor653/3198 CPU: 1 PID: 3198 Comm: syz-executor653 Not tainted 6.2.0-rc59af4eaa31c1f6c00c8f1e448ed99a45c66340dd5 #6 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014 Call Trace: <TASK> dump_stack_lvl+0x6e/0x91 print_report+0x16a/0x46f kasan_report+0xad/0x130 __token_bucket_busy+0x253/0x260 mptcp_token_new_connect+0x13d/0x490 mptcp_connect+0x4ed/0x860 __inet_stream_connect+0x80e/0xd90 tcp_sendmsg_fastopen+0x3ce/0x710 mptcp_sendmsg+0xff1/0x1a20 inet_sendmsg+0x11d/0x140 __sys_sendto+0x405/0x490 __x64_sys_sendto+0xdc/0x1b0 do_syscall_64+0x3b/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc We need to properly clean-up all the paired MPTCP-level resources and be sure to release the msk last, even when the unaccepted subflow is destroyed by the TCP internals via inet_child_forget(). We can re-use the existing MPTCP_WORK_CLOSE_SUBFLOW infra, explicitly checking that for the critical scenario: the closed subflow is the MPC one, the msk is not accepted and eventually going through full cleanup. With such change, __mptcp_destroy_sock() is always called on msk sockets, even on accepted ones. We don't need anymore to transiently drop one sk reference at msk clone time. Please note this commit depends on the parent one: mptcp: refactor passive socket initialization Fixes: 58b0991 ("mptcp: create msk early") Cc: stable@vger.kernel.org Reported-and-tested-by: Christoph Paasch <cpaasch@apple.com> Closes: multipath-tcp/mptcp_net-next#347 Signed-off-by: Paolo Abeni <pabeni@redhat.com> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> Signed-off-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> Signed-off-by: Sasha Levin <sashal@kernel.org>
1 parent 1516ddb commit 2827f09

3 files changed

Lines changed: 47 additions & 16 deletions

File tree

‎net/mptcp/protocol.c‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2357,15 +2357,27 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
23572357
goto out;
23582358
}
23592359

2360-
sock_orphan(ssk);
23612360
subflow->disposable = 1;
23622361

23632362
/* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops
23642363
* the ssk has been already destroyed, we just need to release the
23652364
* reference owned by msk;
23662365
*/
23672366
if (!inet_csk(ssk)->icsk_ulp_ops) {
2367+
WARN_ON_ONCE(!sock_flag(ssk, SOCK_DEAD));
23682368
kfree_rcu(subflow, rcu);
2369+
} else if (msk->in_accept_queue && msk->first == ssk) {
2370+
/* if the first subflow moved to a close state, e.g. due to
2371+
* incoming reset and we reach here before inet_child_forget()
2372+
* the TCP stack could later try to close it via
2373+
* inet_csk_listen_stop(), or deliver it to the user space via
2374+
* accept().
2375+
* We can't delete the subflow - or risk a double free - nor let
2376+
* the msk survive - or will be leaked in the non accept scenario:
2377+
* fallback and let TCP cope with the subflow cleanup.
2378+
*/
2379+
WARN_ON_ONCE(sock_flag(ssk, SOCK_DEAD));
2380+
mptcp_subflow_drop_ctx(ssk);
23692381
} else {
23702382
/* otherwise tcp will dispose of the ssk and subflow ctx */
23712383
if (ssk->sk_state == TCP_LISTEN) {
@@ -2412,9 +2424,10 @@ static unsigned int mptcp_sync_mss(struct sock *sk, u32 pmtu)
24122424
return 0;
24132425
}
24142426

2415-
static void __mptcp_close_subflow(struct mptcp_sock *msk)
2427+
static void __mptcp_close_subflow(struct sock *sk)
24162428
{
24172429
struct mptcp_subflow_context *subflow, *tmp;
2430+
struct mptcp_sock *msk = mptcp_sk(sk);
24182431

24192432
might_sleep();
24202433

@@ -2428,7 +2441,15 @@ static void __mptcp_close_subflow(struct mptcp_sock *msk)
24282441
if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
24292442
continue;
24302443

2431-
mptcp_close_ssk((struct sock *)msk, ssk, subflow);
2444+
mptcp_close_ssk(sk, ssk, subflow);
2445+
}
2446+
2447+
/* if the MPC subflow has been closed before the msk is accepted,
2448+
* msk will never be accept-ed, close it now
2449+
*/
2450+
if (!msk->first && msk->in_accept_queue) {
2451+
sock_set_flag(sk, SOCK_DEAD);
2452+
inet_sk_state_store(sk, TCP_CLOSE);
24322453
}
24332454
}
24342455

@@ -2637,6 +2658,9 @@ static void mptcp_worker(struct work_struct *work)
26372658
__mptcp_check_send_data_fin(sk);
26382659
mptcp_check_data_fin(sk);
26392660

2661+
if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
2662+
__mptcp_close_subflow(sk);
2663+
26402664
/* There is no point in keeping around an orphaned sk timedout or
26412665
* closed, but we need the msk around to reply to incoming DATA_FIN,
26422666
* even if it is orphaned and in FIN_WAIT2 state
@@ -2652,9 +2676,6 @@ static void mptcp_worker(struct work_struct *work)
26522676
}
26532677
}
26542678

2655-
if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
2656-
__mptcp_close_subflow(msk);
2657-
26582679
if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
26592680
__mptcp_retrans(sk);
26602681

@@ -3084,6 +3105,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
30843105
msk->local_key = subflow_req->local_key;
30853106
msk->token = subflow_req->token;
30863107
msk->subflow = NULL;
3108+
msk->in_accept_queue = 1;
30873109
WRITE_ONCE(msk->fully_established, false);
30883110
if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
30893111
WRITE_ONCE(msk->csum_enabled, true);
@@ -3110,8 +3132,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
31103132
security_inet_csk_clone(nsk, req);
31113133
bh_unlock_sock(nsk);
31123134

3113-
/* keep a single reference */
3114-
__sock_put(nsk);
3135+
/* note: the newly allocated socket refcount is 2 now */
31153136
return nsk;
31163137
}
31173138

@@ -3167,8 +3188,6 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
31673188
goto out;
31683189
}
31693190

3170-
/* acquire the 2nd reference for the owning socket */
3171-
sock_hold(new_mptcp_sock);
31723191
newsk = new_mptcp_sock;
31733192
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEPASSIVEACK);
31743193
} else {
@@ -3726,6 +3745,8 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
37263745
struct mptcp_subflow_context *subflow;
37273746
struct sock *newsk = newsock->sk;
37283747

3748+
msk->in_accept_queue = 0;
3749+
37293750
lock_sock(newsk);
37303751

37313752
/* set ssk->sk_socket of accept()ed flows to mptcp socket.

‎net/mptcp/protocol.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,8 @@ struct mptcp_sock {
286286
u8 recvmsg_inq:1,
287287
cork:1,
288288
nodelay:1,
289-
fastopening:1;
289+
fastopening:1,
290+
in_accept_queue:1;
290291
int connect_flags;
291292
struct work_struct work;
292293
struct sk_buff *ooo_last_skb;
@@ -651,6 +652,8 @@ void mptcp_subflow_set_active(struct mptcp_subflow_context *subflow);
651652

652653
bool mptcp_subflow_active(struct mptcp_subflow_context *subflow);
653654

655+
void mptcp_subflow_drop_ctx(struct sock *ssk);
656+
654657
static inline void mptcp_subflow_tcp_fallback(struct sock *sk,
655658
struct mptcp_subflow_context *ctx)
656659
{

‎net/mptcp/subflow.c‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -636,9 +636,10 @@ static bool subflow_hmac_valid(const struct request_sock *req,
636636

637637
static void mptcp_force_close(struct sock *sk)
638638
{
639-
/* the msk is not yet exposed to user-space */
639+
/* the msk is not yet exposed to user-space, and refcount is 2 */
640640
inet_sk_state_store(sk, TCP_CLOSE);
641641
sk_common_release(sk);
642+
sock_put(sk);
642643
}
643644

644645
static void subflow_ulp_fallback(struct sock *sk,
@@ -654,7 +655,7 @@ static void subflow_ulp_fallback(struct sock *sk,
654655
mptcp_subflow_ops_undo_override(sk);
655656
}
656657

657-
static void subflow_drop_ctx(struct sock *ssk)
658+
void mptcp_subflow_drop_ctx(struct sock *ssk)
658659
{
659660
struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(ssk);
660661

@@ -758,7 +759,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
758759

759760
if (new_msk)
760761
mptcp_copy_inaddrs(new_msk, child);
761-
subflow_drop_ctx(child);
762+
mptcp_subflow_drop_ctx(child);
762763
goto out;
763764
}
764765

@@ -849,7 +850,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
849850
return child;
850851

851852
dispose_child:
852-
subflow_drop_ctx(child);
853+
mptcp_subflow_drop_ctx(child);
853854
tcp_rsk(req)->drop_req = true;
854855
inet_csk_prepare_for_destroy_sock(child);
855856
tcp_done(child);
@@ -1804,7 +1805,6 @@ void mptcp_subflow_queue_clean(struct sock *listener_sk, struct sock *listener_s
18041805
struct sock *sk = (struct sock *)msk;
18051806
bool do_cancel_work;
18061807

1807-
sock_hold(sk);
18081808
lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
18091809
next = msk->dl_next;
18101810
msk->first = NULL;
@@ -1892,6 +1892,13 @@ static void subflow_ulp_release(struct sock *ssk)
18921892
* when the subflow is still unaccepted
18931893
*/
18941894
release = ctx->disposable || list_empty(&ctx->node);
1895+
1896+
/* inet_child_forget() does not call sk_state_change(),
1897+
* explicitly trigger the socket close machinery
1898+
*/
1899+
if (!release && !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW,
1900+
&mptcp_sk(sk)->flags))
1901+
mptcp_schedule_work(sk);
18951902
sock_put(sk);
18961903
}
18971904

0 commit comments

Comments
 (0)