[PATCH mptcp 0/2] mptcp: fix crash with mptcp-ulp on tcp sockets

[PATCH mptcp 0/2] mptcp: fix crash with mptcp-ulp on tcp sockets

[Florian Westphal]

While working on the tls-ULP syzbot report I found that its also
possible to set the "mptcp" ulp from userspace, iff the socket is a
tcp socket returned via accept() on an mptcp listen socket.

First patch fixes this, second patch adds a test case.

Florian Westphal (2):
  mptcp: clear 'kern' flag from fallback sockets
  selftests: mptcp: try to set mptcp ulp mode in different sk states

 net/mptcp/protocol.c                          |  1 +
 .../selftests/net/mptcp/mptcp_connect.c       | 97 ++++++++++---------
 .../selftests/net/mptcp/mptcp_connect.sh      | 20 ----
 3 files changed, 52 insertions(+), 66 deletions(-)

-- 
2.32.0

[PATCH mptcp 1/2] mptcp: clear 'kern' flag from fallback sockets

[Florian Westphal]

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;
 }
 
-- 
2.32.0

[PATCH mptcp 2/2] selftests: mptcp: try to set mptcp ulp mode in different sk states

[Florian Westphal]

The kernel will crash without
'mptcp: clear 'kern' flag from fallback sockets' change.

Since this doesn't slow down testing in a noticeable way,
run this unconditionally.

The explicit test did not catch this, because the check was done
for tcp socket returned by 'socket(.. IPPROTO_TCP) rather than a
tcp socket returned by accept() on a mptcp listen fd.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 .../selftests/net/mptcp/mptcp_connect.c       | 97 ++++++++++---------
 .../selftests/net/mptcp/mptcp_connect.sh      | 20 ----
 2 files changed, 51 insertions(+), 66 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index ffdf7bbc16af..8628aa61b763 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -61,7 +61,6 @@ static enum cfg_peek cfg_peek = CFG_NONE_PEEK;
 static const char *cfg_host;
 static const char *cfg_port	= "12000";
 static int cfg_sock_proto	= IPPROTO_MPTCP;
-static bool tcpulp_audit;
 static int pf = AF_INET;
 static int cfg_sndbuf;
 static int cfg_rcvbuf;
@@ -120,7 +119,6 @@ static void die_usage(void)
 	fprintf(stderr, "\t-R num -- set SO_RCVBUF to num\n");
 	fprintf(stderr, "\t-s [MPTCP|TCP] -- use mptcp(default) or tcp sockets\n");
 	fprintf(stderr, "\t-S num -- set SO_SNDBUF to num\n");
-	fprintf(stderr, "\t-u -- check mptcp ulp\n");
 	fprintf(stderr, "\t-w num -- wait num sec before closing the socket\n");
 	exit(1);
 }
@@ -228,6 +226,42 @@ static void set_transparent(int fd, int pf)
 	}
 }
 
+static int do_ulp_so(int sock, const char *name)
+{
+	return setsockopt(sock, IPPROTO_TCP, TCP_ULP, name, strlen(name));
+}
+
+#define X(m)	xerror("%s:%u: %s: failed for proto %d at line %u", __FILE__, __LINE__, (m), proto, line)
+static void sock_test_tcpulp(int sock, int proto, unsigned int line)
+{
+	socklen_t buflen = 8;
+	char buf[8] = "";
+	int ret = getsockopt(sock, IPPROTO_TCP, TCP_ULP, buf, &buflen);
+
+	if (ret != 0)
+		X("getsockopt");
+
+	if (buflen > 0) {
+		if (strcmp(buf, "mptcp") != 0)
+			xerror("unexpected ULP '%s' for proto %d at line %u", buf, proto, line);
+		ret = do_ulp_so(sock, "tls");
+		if (ret == 0)
+			X("setsockopt");
+	} else if (proto == IPPROTO_MPTCP) {
+		ret = do_ulp_so(sock, "tls");
+		if (ret != -1)
+			X("setsockopt");
+	}
+
+	ret = do_ulp_so(sock, "mptcp");
+	if (ret != -1)
+		X("setsockopt");
+
+#undef X
+}
+
+#define SOCK_TEST_TCPULP(s, p) sock_test_tcpulp((s), (p), __LINE__)
+
 static int sock_listen_mptcp(const char * const listenaddr,
 			     const char * const port)
 {
@@ -251,6 +285,8 @@ static int sock_listen_mptcp(const char * const listenaddr,
 		if (sock < 0)
 			continue;
 
+		SOCK_TEST_TCPULP(sock, cfg_sock_proto);
+
 		if (-1 == setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &one,
 				     sizeof(one)))
 			perror("setsockopt");
@@ -273,50 +309,17 @@ static int sock_listen_mptcp(const char * const listenaddr,
 		return sock;
 	}
 
+	SOCK_TEST_TCPULP(sock, cfg_sock_proto);
+
 	if (listen(sock, 20)) {
 		perror("listen");
 		close(sock);
 		return -1;
 	}
 
-	return sock;
-}
+	SOCK_TEST_TCPULP(sock, cfg_sock_proto);
 
-static bool sock_test_tcpulp(const char * const remoteaddr,
-			     const char * const port)
-{
-	struct addrinfo hints = {
-		.ai_protocol = IPPROTO_TCP,
-		.ai_socktype = SOCK_STREAM,
-	};
-	struct addrinfo *a, *addr;
-	int sock = -1, ret = 0;
-	bool test_pass = false;
-
-	hints.ai_family = AF_INET;
-
-	xgetaddrinfo(remoteaddr, port, &hints, &addr);
-	for (a = addr; a; a = a->ai_next) {
-		sock = socket(a->ai_family, a->ai_socktype, IPPROTO_TCP);
-		if (sock < 0) {
-			perror("socket");
-			continue;
-		}
-		ret = setsockopt(sock, IPPROTO_TCP, TCP_ULP, "mptcp",
-				 sizeof("mptcp"));
-		if (ret == -1 && errno == EOPNOTSUPP)
-			test_pass = true;
-		close(sock);
-
-		if (test_pass)
-			break;
-		if (!ret)
-			fprintf(stderr,
-				"setsockopt(TCP_ULP) returned 0\n");
-		else
-			perror("setsockopt(TCP_ULP)");
-	}
-	return test_pass;
+	return sock;
 }
 
 static int sock_connect_mptcp(const char * const remoteaddr,
@@ -340,6 +343,8 @@ static int sock_connect_mptcp(const char * const remoteaddr,
 			continue;
 		}
 
+		SOCK_TEST_TCPULP(sock, proto);
+
 		if (cfg_mark)
 			set_mark(sock, cfg_mark);
 
@@ -354,6 +359,8 @@ static int sock_connect_mptcp(const char * const remoteaddr,
 	}
 
 	freeaddrinfo(addr);
+	if (sock != -1)
+		SOCK_TEST_TCPULP(sock, proto);
 	return sock;
 }
 
@@ -983,6 +990,8 @@ int main_loop_s(int listensock)
 				xerror("can't open %s: %d", cfg_input, errno);
 		}
 
+		SOCK_TEST_TCPULP(remotesock, 0);
+
 		copyfd_io(fd, remotesock, 1, true);
 	} else {
 		perror("accept");
@@ -1127,6 +1136,8 @@ int main_loop(void)
 again:
 	check_getpeername_connect(fd);
 
+	SOCK_TEST_TCPULP(fd, cfg_sock_proto);
+
 	if (cfg_rcvbuf)
 		set_rcvbuf(fd, cfg_rcvbuf);
 	if (cfg_sndbuf)
@@ -1243,7 +1254,7 @@ static void parse_opts(int argc, char **argv)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "6c:hi:I:jlm:M:o:p:P:r:R:s:S:t:T:uw:")) != -1) {
+	while ((c = getopt(argc, argv, "6c:hi:I:jlm:M:o:p:P:r:R:s:S:t:T:w:")) != -1) {
 		switch (c) {
 		case 'j':
 			cfg_join = true;
@@ -1275,9 +1286,6 @@ static void parse_opts(int argc, char **argv)
 		case 'h':
 			die_usage();
 			break;
-		case 'u':
-			tcpulp_audit = true;
-			break;
 		case '6':
 			pf = AF_INET6;
 			break;
@@ -1331,9 +1339,6 @@ int main(int argc, char *argv[])
 	signal(SIGUSR1, handle_signal);
 	parse_opts(argc, argv);
 
-	if (tcpulp_audit)
-		return sock_test_tcpulp(cfg_host, cfg_port) ? 0 : 1;
-
 	if (listen_mode) {
 		int fd = sock_listen_mptcp(cfg_host, cfg_port);
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index de6c630a59da..cb5809b89081 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -301,24 +301,6 @@ check_mptcp_disabled()
 	return 0
 }
 
-check_mptcp_ulp_setsockopt()
-{
-	local t retval
-	t="ns_ulp-$sech-$(mktemp -u XXXXXX)"
-
-	ip netns add ${t} || exit $ksft_skip
-	if ! ip netns exec ${t} ./mptcp_connect -u -p 10000 -s TCP 127.0.0.1 2>&1; then
-		printf "setsockopt(..., TCP_ULP, \"mptcp\", ...) allowed\t[ FAIL ]\n"
-		retval=1
-		ret=$retval
-	else
-		printf "setsockopt(..., TCP_ULP, \"mptcp\", ...) blocked\t[ OK ]\n"
-		retval=0
-	fi
-	ip netns del ${t}
-	return $retval
-}
-
 # $1: IP address
 is_v6()
 {
@@ -812,8 +794,6 @@ make_file "$sin" "server"
 
 check_mptcp_disabled
 
-check_mptcp_ulp_setsockopt
-
 stop_if_error "The kernel configuration is not valid for MPTCP"
 
 echo "INFO: validating network environment with pings"
-- 
2.32.0

Re: [PATCH mptcp 1/2] mptcp: clear 'kern' flag from fallback sockets

[Mat Martineau]

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

[PATCH v2 1/2] mptcp: clear 'kern' flag from fallback sockets

[Florian Westphal]

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>
---
 v2: also handle early-return

 net/mptcp/protocol.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8319e601bc2d..4a8f2476cc75 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3013,7 +3013,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
 		 */
 		if (WARN_ON_ONCE(!new_mptcp_sock)) {
 			tcp_sk(newsk)->is_mptcp = 0;
-			return newsk;
+			goto out;
 		}
 
 		/* acquire the 2nd reference for the owning socket */
@@ -3025,6 +3025,8 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
 				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
 	}
 
+out:
+	newsk->sk_kern_sock = kern;
 	return newsk;
 }
 
-- 
2.32.0

Re: [PATCH v2 1/2] mptcp: clear 'kern' flag from fallback sockets

[Mat Martineau]

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>
> ---
> v2: also handle early-return

Thanks - v2 looks good to me.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

>
> net/mptcp/protocol.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 8319e601bc2d..4a8f2476cc75 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3013,7 +3013,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
> 		 */
> 		if (WARN_ON_ONCE(!new_mptcp_sock)) {
> 			tcp_sk(newsk)->is_mptcp = 0;
> -			return newsk;
> +			goto out;
> 		}
>
> 		/* acquire the 2nd reference for the owning socket */
> @@ -3025,6 +3025,8 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
> 				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
> 	}
>
> +out:
> +	newsk->sk_kern_sock = kern;
> 	return newsk;
> }
>
> -- 
> 2.32.0
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp 0/2] mptcp: fix crash with mptcp-ulp on tcp sockets

[Matthieu Baerts]

Hi Florian, Mat,

On 06/12/2021 16:51, Florian Westphal wrote:
> While working on the tls-ULP syzbot report I found that its also
> possible to set the "mptcp" ulp from userspace, iff the socket is a
> tcp socket returned via accept() on an mptcp listen socket.
> 
> First patch fixes this, second patch adds a test case.
>
> Florian Westphal (2):
>   mptcp: clear 'kern' flag from fallback sockets
>   selftests: mptcp: try to set mptcp ulp mode in different sk states

Thank you for the patches and the reviews!

- cf6bfb9af34f: mptcp: clear 'kern' flag from fallback sockets
 (v2)
- Results: 4be3d8d5b45c..23aff1c44f69

- 9a7f7dc671fc: selftests: mptcp: try to set mptcp ulp mode in different
sk states

- Results: 23aff1c44f69..1250e74665ba

Builds and tests are now in progress:



https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20211207T135708

https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH v2 1/2] mptcp: clear 'kern' flag from fallback sockets

[Mat Martineau]

On Mon, 6 Dec 2021, Mat Martineau wrote:

> 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>
>> ---
>> v2: also handle early-return
>
> Thanks - v2 looks good to me.
>
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
>> 
>> net/mptcp/protocol.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 8319e601bc2d..4a8f2476cc75 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -3013,7 +3013,7 @@ static struct sock *mptcp_accept(struct sock *sk, int 
>> flags, int *err,
>> 		 */
>> 		if (WARN_ON_ONCE(!new_mptcp_sock)) {
>> 			tcp_sk(newsk)->is_mptcp = 0;
>> -			return newsk;
>> +			goto out;
>> 		}
>>
>> 		/* acquire the 2nd reference for the owning socket */
>> @@ -3025,6 +3025,8 @@ static struct sock *mptcp_accept(struct sock *sk, int 
>> flags, int *err,
>> 				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
>> 	}
>> 
>> +out:
>> +	newsk->sk_kern_sock = kern;

Florian -

I was about to upstream this for -net, but have another question first.

Is there anything else in newsk that needs to be updated when changing 
sk_kern_sock? sk_alloc() handles some reference counts differently for 
kern socks, and sock_lock_init() sets things up differently for lockdep.


>> 	return newsk;
>> }
>> 
>> -- 
>> 2.32.0
>> 
>> 
>> 
>
> --
> Mat Martineau
> Intel
>
>

--
Mat Martineau
Intel

Re: [PATCH v2 1/2] mptcp: clear 'kern' flag from fallback sockets

[Florian Westphal]

Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
> On Mon, 6 Dec 2021, Mat Martineau wrote:
> 
> > 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>
> > > ---
> > > v2: also handle early-return
> > 
> > Thanks - v2 looks good to me.
> > 
> > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> > 
> > > 
> > > net/mptcp/protocol.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 8319e601bc2d..4a8f2476cc75 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -3013,7 +3013,7 @@ static struct sock *mptcp_accept(struct sock
> > > *sk, int flags, int *err,
> > > 		 */
> > > 		if (WARN_ON_ONCE(!new_mptcp_sock)) {
> > > 			tcp_sk(newsk)->is_mptcp = 0;
> > > -			return newsk;
> > > +			goto out;
> > > 		}
> > > 
> > > 		/* acquire the 2nd reference for the owning socket */
> > > @@ -3025,6 +3025,8 @@ static struct sock *mptcp_accept(struct sock
> > > *sk, int flags, int *err,
> > > 				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
> > > 	}
> > > 
> > > +out:
> > > +	newsk->sk_kern_sock = kern;
> 
> Florian -
> 
> I was about to upstream this for -net, but have another question first.
> 
> Is there anything else in newsk that needs to be updated when changing
> sk_kern_sock? sk_alloc() handles some reference counts differently for kern
> socks, and sock_lock_init() sets things up differently for lockdep.

AFAICS no.

The tcpsk inherits these settings from its parent (listen) sk, so they
always have 'kern = 1'.

Even before this change, lock depclass is not correct (kernel, not user).

Need to export code from core to change this.

The netns refcount bump is not needed, but at this point it has already
happened so even if we undo+clear ->sk_net_refcnt it won't buy anthing.

So only alternative I see is to toss this patch and use a different
sk marker to block mptcp ulp on normal tcp sockets.

This would not change the incorrect lockdep class in this case of course
but would avoid messing with this.

tp->is_mptcp would come to mind, we only need to set it to 1 before
adding the mptcp ulp from inside the kernel rather than in the mptcp ulp
init function.

Re: [PATCH v2 1/2] mptcp: clear 'kern' flag from fallback sockets

[Paolo Abeni]

On Fri, 2021-12-10 at 10:00 +0100, Florian Westphal wrote:
> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
> > On Mon, 6 Dec 2021, Mat Martineau wrote:
> > 
> > > 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>
> > > > ---
> > > > v2: also handle early-return
> > > 
> > > Thanks - v2 looks good to me.
> > > 
> > > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> > > 
> > > > 
> > > > net/mptcp/protocol.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > > index 8319e601bc2d..4a8f2476cc75 100644
> > > > --- a/net/mptcp/protocol.c
> > > > +++ b/net/mptcp/protocol.c
> > > > @@ -3013,7 +3013,7 @@ static struct sock *mptcp_accept(struct sock
> > > > *sk, int flags, int *err,
> > > > 		 */
> > > > 		if (WARN_ON_ONCE(!new_mptcp_sock)) {
> > > > 			tcp_sk(newsk)->is_mptcp = 0;
> > > > -			return newsk;
> > > > +			goto out;
> > > > 		}
> > > > 
> > > > 		/* acquire the 2nd reference for the owning socket */
> > > > @@ -3025,6 +3025,8 @@ static struct sock *mptcp_accept(struct sock
> > > > *sk, int flags, int *err,
> > > > 				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
> > > > 	}
> > > > 
> > > > +out:
> > > > +	newsk->sk_kern_sock = kern;
> > 
> > Florian -
> > 
> > I was about to upstream this for -net, but have another question first.
> > 
> > Is there anything else in newsk that needs to be updated when changing
> > sk_kern_sock? sk_alloc() handles some reference counts differently for kern
> > socks, and sock_lock_init() sets things up differently for lockdep.
> 
> AFAICS no.
> 
> The tcpsk inherits these settings from its parent (listen) sk, so they
> always have 'kern = 1'.
> 
> Even before this change, lock depclass is not correct (kernel, not user).
> 
> Need to export code from core to change this.

I personally would go this way, with a separate patch, possibly addinig
a new helper for that.

Somewhat related: I don't see where the lockdep class for
sk_callback_lock is set properly for any in-kernel user doing accept()
on plain TCP socket (I mean: not an mptcp listener!). sk_clone_lock()
calls sk_init_common() which uses unconditionally the user-space
lockdep class. ?!?

Cheers,

Paolo

Re: [PATCH v2 1/2] mptcp: clear 'kern' flag from fallback sockets

[Mat Martineau]

On Fri, 10 Dec 2021, Paolo Abeni wrote:

> On Fri, 2021-12-10 at 10:00 +0100, Florian Westphal wrote:
>> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
>>> On Mon, 6 Dec 2021, Mat Martineau wrote:
>>>
>>>> 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>
>>>>> ---
>>>>> v2: also handle early-return
>>>>
>>>> Thanks - v2 looks good to me.
>>>>
>>>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>>>
>>>>>
>>>>> net/mptcp/protocol.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>>> index 8319e601bc2d..4a8f2476cc75 100644
>>>>> --- a/net/mptcp/protocol.c
>>>>> +++ b/net/mptcp/protocol.c
>>>>> @@ -3013,7 +3013,7 @@ static struct sock *mptcp_accept(struct sock
>>>>> *sk, int flags, int *err,
>>>>> 		 */
>>>>> 		if (WARN_ON_ONCE(!new_mptcp_sock)) {
>>>>> 			tcp_sk(newsk)->is_mptcp = 0;
>>>>> -			return newsk;
>>>>> +			goto out;
>>>>> 		}
>>>>>
>>>>> 		/* acquire the 2nd reference for the owning socket */
>>>>> @@ -3025,6 +3025,8 @@ static struct sock *mptcp_accept(struct sock
>>>>> *sk, int flags, int *err,
>>>>> 				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
>>>>> 	}
>>>>>
>>>>> +out:
>>>>> +	newsk->sk_kern_sock = kern;
>>>
>>> Florian -
>>>
>>> I was about to upstream this for -net, but have another question first.
>>>
>>> Is there anything else in newsk that needs to be updated when changing
>>> sk_kern_sock? sk_alloc() handles some reference counts differently for kern
>>> socks, and sock_lock_init() sets things up differently for lockdep.
>>
>> AFAICS no.
>>
>> The tcpsk inherits these settings from its parent (listen) sk, so they
>> always have 'kern = 1'.
>>
>> Even before this change, lock depclass is not correct (kernel, not user).
>>
>> Need to export code from core to change this.
>
> I personally would go this way, with a separate patch, possibly addinig
> a new helper for that.
>

Are you thinking that would be cleanup for net-next? Or urgent enough for 
-net?

I lean toward net-next, given the likely backporting of this fix.

> Somewhat related: I don't see where the lockdep class for
> sk_callback_lock is set properly for any in-kernel user doing accept()
> on plain TCP socket (I mean: not an mptcp listener!). sk_clone_lock()
> calls sk_init_common() which uses unconditionally the user-space
> lockdep class. ?!?
>

Yeah - af_kern_callback_keys is only referenced in sock_init_data(), which 
always inits the lockdep class for sk_callback_lock for userspace first by 
calling sk_init_common(), then always calls lockdep_set_class_and_name() a 
second time for sk_callback_lock (setting appropriately for kern or 
userspace).

--
Mat Martineau
Intel

Re: [PATCH v2 1/2] mptcp: clear 'kern' flag from fallback sockets

[Mat Martineau]

On Fri, 10 Dec 2021, Florian Westphal wrote:

> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
>> On Mon, 6 Dec 2021, Mat Martineau wrote:
>>
>>> 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>
>>>> ---
>>>> v2: also handle early-return
>>>
>>> Thanks - v2 looks good to me.
>>>
>>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>>
>>>>
>>>> net/mptcp/protocol.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>> index 8319e601bc2d..4a8f2476cc75 100644
>>>> --- a/net/mptcp/protocol.c
>>>> +++ b/net/mptcp/protocol.c
>>>> @@ -3013,7 +3013,7 @@ static struct sock *mptcp_accept(struct sock
>>>> *sk, int flags, int *err,
>>>> 		 */
>>>> 		if (WARN_ON_ONCE(!new_mptcp_sock)) {
>>>> 			tcp_sk(newsk)->is_mptcp = 0;
>>>> -			return newsk;
>>>> +			goto out;
>>>> 		}
>>>>
>>>> 		/* acquire the 2nd reference for the owning socket */
>>>> @@ -3025,6 +3025,8 @@ static struct sock *mptcp_accept(struct sock
>>>> *sk, int flags, int *err,
>>>> 				MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
>>>> 	}
>>>>
>>>> +out:
>>>> +	newsk->sk_kern_sock = kern;
>>
>> Florian -
>>
>> I was about to upstream this for -net, but have another question first.
>>
>> Is there anything else in newsk that needs to be updated when changing
>> sk_kern_sock? sk_alloc() handles some reference counts differently for kern
>> socks, and sock_lock_init() sets things up differently for lockdep.
>
> AFAICS no.
>
> The tcpsk inherits these settings from its parent (listen) sk, so they
> always have 'kern = 1'.
>
> Even before this change, lock depclass is not correct (kernel, not user).
>
> Need to export code from core to change this.
>
> The netns refcount bump is not needed, but at this point it has already
> happened so even if we undo+clear ->sk_net_refcnt it won't buy anthing.
>

Ok, thanks for the background on the refcounts. I also now see the code in 
mtpcp_subflow_create_socket() that already adjusts the refcounts.

> So only alternative I see is to toss this patch and use a different
> sk marker to block mptcp ulp on normal tcp sockets.
>
> This would not change the incorrect lockdep class in this case of course
> but would avoid messing with this.
>
> tp->is_mptcp would come to mind, we only need to set it to 1 before
> adding the mptcp ulp from inside the kernel rather than in the mptcp ulp
> init function.
>

So the question is which inconsistency is better: mismatch between the 
lockdep class and sk_kern_sock bit (the original patch for this email 
thread), or having a sk_kern_sock=1 socket out in usespace (the proposed 
alternative).

Neither seems ideal, but also don't appear to have serious consequences. 
For a -net fix now, this patch (clearing the kern bit) seems like the most 
straightforward for backporting. The lockdep fix could be handled 
independently, as it's a separate existing issue?


I will plan to upstream the existing patches from the export branch on 
Monday if there's no objection posted here!


--
Mat Martineau
Intel