| Message ID | 20211206155120.26929-2-fw@strlen.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Commit | cf6bfb9af34f91c85eb70fd33d6fbb43f469d374 |
| Headers | show |
| Series | mptcp: fix crash with mptcp-ulp on tcp sockets | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| matttbe/KVM_Validation__normal | warning | Unstable: 1 failed test(s): selftest_mptcp_join |
| matttbe/KVM_Validation__debug | warning | Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join |
On Mon, 6 Dec 2021, Florian Westphal wrote: > The mptcp ULP extension relies on sk->sk_sock_kern being set correctly: > It prevents setsockopt(fd, IPPROTO_TCP, TCP_ULP, "mptcp", 6); from > working for plain tcp sockets (any userspace-exposed socket). > > But in case of fallback, accept() can return a plain tcp sk. > In such case, sk is still tagged as 'kernel' and setsockopt will work. > > This will crash the kernel, The subflow extension has a NULL ctx->conn > mptcp socket: > > BUG: KASAN: null-ptr-deref in subflow_data_ready+0x181/0x2b0 > Call Trace: > tcp_data_ready+0xf8/0x370 > [..] > > Fixes: cf7da0d66cc1 ("mptcp: Create SUBFLOW socket for incoming connections") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/mptcp/protocol.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 8319e601bc2d..34ea4b25128e 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -3025,6 +3025,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err, > MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK); > } > > + newsk->sk_kern_sock = kern; > return newsk; > } Florian - There's an early return in this function where the newsk from inet_csk_accept() is also used. From the WARN_ON_ONCE() for that return, it shouldn't happen, and changes to subflow_syn_recv_sock() appear to make it impossible and therefore dead code. Could do one of these: 1. Set sk_kern_sock for the early return for this -net fix, delete the dead code path in mptcp-next if needed / agreed upon 2. Delete the early return now Option 1 seems like the safer approach for -net, do you agree? -- Mat Martineau Intel
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 8319e601bc2d..34ea4b25128e 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -3025,6 +3025,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err, MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK); } + newsk->sk_kern_sock = kern; return newsk; }
The mptcp ULP extension relies on sk->sk_sock_kern being set correctly: It prevents setsockopt(fd, IPPROTO_TCP, TCP_ULP, "mptcp", 6); from working for plain tcp sockets (any userspace-exposed socket). But in case of fallback, accept() can return a plain tcp sk. In such case, sk is still tagged as 'kernel' and setsockopt will work. This will crash the kernel, The subflow extension has a NULL ctx->conn mptcp socket: BUG: KASAN: null-ptr-deref in subflow_data_ready+0x181/0x2b0 Call Trace: tcp_data_ready+0xf8/0x370 [..] Fixes: cf7da0d66cc1 ("mptcp: Create SUBFLOW socket for incoming connections") Signed-off-by: Florian Westphal <fw@strlen.de> --- net/mptcp/protocol.c | 1 + 1 file changed, 1 insertion(+)