[MPTCP] [PATCH net] mptcp: be careful on subflow creation

[MPTCP] [PATCH net] mptcp: be careful on subflow creation

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

Nicolas reported the following oops:

[ 1521.392541] BUG: kernel NULL pointer dereference, address: 00000000000000c0
[ 1521.394189] #PF: supervisor read access in kernel mode
[ 1521.395376] #PF: error_code(0x0000) - not-present page
[ 1521.396607] PGD 0 P4D 0
[ 1521.397156] Oops: 0000 [#1] SMP PTI
[ 1521.398020] CPU: 0 PID: 22986 Comm: kworker/0:2 Not tainted 5.8.0-rc4+ #109
[ 1521.399618] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 1521.401728] Workqueue: events mptcp_worker
[ 1521.402651] RIP: 0010:mptcp_subflow_create_socket+0xf1/0x1c0
[ 1521.403954] Code: 24 08 89 44 24 04 48 8b 7a 18 e8 2a 48 d4 ff 8b 44 24 04 85 c0 75 7a 48 8b 8b 78 02 00 00 48 8b 54 24 08 48 8d bb 80 00 00 00 <48> 8b 89 c0 00 00 00 48 89 8a c0 00 00 00 48 8b 8b 78 02 00 00 8b
[ 1521.408201] RSP: 0000:ffffabc4002d3c60 EFLAGS: 00010246
[ 1521.409433] RAX: 0000000000000000 RBX: ffffa0b9ad8c9a00 RCX: 0000000000000000
[ 1521.411096] RDX: ffffa0b9ae78a300 RSI: 00000000fffffe01 RDI: ffffa0b9ad8c9a80
[ 1521.412734] RBP: ffffa0b9adff2e80 R08: ffffa0b9af02d640 R09: ffffa0b9ad923a00
[ 1521.414333] R10: ffffabc4007139f8 R11: fefefefefefefeff R12: ffffabc4002d3cb0
[ 1521.415918] R13: ffffa0b9ad91fa58 R14: ffffa0b9ad8c9f9c R15: 0000000000000000
[ 1521.417592] FS:  0000000000000000(0000) GS:ffffa0b9af000000(0000) knlGS:0000000000000000
[ 1521.419490] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1521.420839] CR2: 00000000000000c0 CR3: 000000002951e006 CR4: 0000000000160ef0
[ 1521.422511] Call Trace:
[ 1521.423103]  __mptcp_subflow_connect+0x94/0x1f0
[ 1521.425376]  mptcp_pm_create_subflow_or_signal_addr+0x200/0x2a0
[ 1521.426736]  mptcp_worker+0x31b/0x390
[ 1521.431324]  process_one_work+0x1fc/0x3f0
[ 1521.432268]  worker_thread+0x2d/0x3b0
[ 1521.434197]  kthread+0x117/0x130
[ 1521.435783]  ret_from_fork+0x22/0x30

on some unconventional configuration.

The MPTCP protocol is trying to create a subflow for an
unaccepted server socket. That is allowed by the RFC, even
if subflow creation will likely fail.
Unaccepted sockets have still a NULL sk_socket field,
avoid the issue by failing earlier.

Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski(a)tessares.net>
Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
 net/mptcp/subflow.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3838a0b3a21f..3c31a8160f19 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1032,6 +1032,12 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
 	struct socket *sf;
 	int err;
 
+	/* un-accepted server sockets can reach here - on bad configuration
+	 * bail early to avoid greater trouble later
+	 */
+	if (unlikely(!sk->sk_socket))
+		return -EINVAL;
+
 	err = sock_create_kern(net, sk->sk_family, SOCK_STREAM, IPPROTO_TCP,
 			       &sf);
 	if (err)
-- 
2.26.2

[PATCH net] mptcp: be careful on subflow creation

[Paolo Abeni]

Nicolas reported the following oops:

[ 1521.392541] BUG: kernel NULL pointer dereference, address: 00000000000000c0
[ 1521.394189] #PF: supervisor read access in kernel mode
[ 1521.395376] #PF: error_code(0x0000) - not-present page
[ 1521.396607] PGD 0 P4D 0
[ 1521.397156] Oops: 0000 [#1] SMP PTI
[ 1521.398020] CPU: 0 PID: 22986 Comm: kworker/0:2 Not tainted 5.8.0-rc4+ #109
[ 1521.399618] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 1521.401728] Workqueue: events mptcp_worker
[ 1521.402651] RIP: 0010:mptcp_subflow_create_socket+0xf1/0x1c0
[ 1521.403954] Code: 24 08 89 44 24 04 48 8b 7a 18 e8 2a 48 d4 ff 8b 44 24 04 85 c0 75 7a 48 8b 8b 78 02 00 00 48 8b 54 24 08 48 8d bb 80 00 00 00 <48> 8b 89 c0 00 00 00 48 89 8a c0 00 00 00 48 8b 8b 78 02 00 00 8b
[ 1521.408201] RSP: 0000:ffffabc4002d3c60 EFLAGS: 00010246
[ 1521.409433] RAX: 0000000000000000 RBX: ffffa0b9ad8c9a00 RCX: 0000000000000000
[ 1521.411096] RDX: ffffa0b9ae78a300 RSI: 00000000fffffe01 RDI: ffffa0b9ad8c9a80
[ 1521.412734] RBP: ffffa0b9adff2e80 R08: ffffa0b9af02d640 R09: ffffa0b9ad923a00
[ 1521.414333] R10: ffffabc4007139f8 R11: fefefefefefefeff R12: ffffabc4002d3cb0
[ 1521.415918] R13: ffffa0b9ad91fa58 R14: ffffa0b9ad8c9f9c R15: 0000000000000000
[ 1521.417592] FS:  0000000000000000(0000) GS:ffffa0b9af000000(0000) knlGS:0000000000000000
[ 1521.419490] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1521.420839] CR2: 00000000000000c0 CR3: 000000002951e006 CR4: 0000000000160ef0
[ 1521.422511] Call Trace:
[ 1521.423103]  __mptcp_subflow_connect+0x94/0x1f0
[ 1521.425376]  mptcp_pm_create_subflow_or_signal_addr+0x200/0x2a0
[ 1521.426736]  mptcp_worker+0x31b/0x390
[ 1521.431324]  process_one_work+0x1fc/0x3f0
[ 1521.432268]  worker_thread+0x2d/0x3b0
[ 1521.434197]  kthread+0x117/0x130
[ 1521.435783]  ret_from_fork+0x22/0x30

on some unconventional configuration.

The MPTCP protocol is trying to create a subflow for an
unaccepted server socket. That is allowed by the RFC, even
if subflow creation will likely fail.
Unaccepted sockets have still a NULL sk_socket field,
avoid the issue by failing earlier.

Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3838a0b3a21f..3c31a8160f19 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1032,6 +1032,12 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
 	struct socket *sf;
 	int err;
 
+	/* un-accepted server sockets can reach here - on bad configuration
+	 * bail early to avoid greater trouble later
+	 */
+	if (unlikely(!sk->sk_socket))
+		return -EINVAL;
+
 	err = sock_create_kern(net, sk->sk_family, SOCK_STREAM, IPPROTO_TCP,
 			       &sf);
 	if (err)
-- 
2.26.2

[MPTCP] Re: [PATCH net] mptcp: be careful on subflow creation

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

Hi Paolo,

On 04/08/2020 18:31, Paolo Abeni wrote:
> Nicolas reported the following oops:

(...)

> on some unconventional configuration.
> 
> The MPTCP protocol is trying to create a subflow for an
> unaccepted server socket. That is allowed by the RFC, even
> if subflow creation will likely fail.
> Unaccepted sockets have still a NULL sk_socket field,
> avoid the issue by failing earlier.
> 
> Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski(a)tessares.net>
> Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")

Thank you for the patch, the addition in the code looks very good to me!

But are you sure the commit you mention introduces the issue you fix here?

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

Re: [PATCH net] mptcp: be careful on subflow creation

[Matthieu Baerts]

Hi Paolo,

On 04/08/2020 18:31, Paolo Abeni wrote:
> Nicolas reported the following oops:

(...)

> on some unconventional configuration.
> 
> The MPTCP protocol is trying to create a subflow for an
> unaccepted server socket. That is allowed by the RFC, even
> if subflow creation will likely fail.
> Unaccepted sockets have still a NULL sk_socket field,
> avoid the issue by failing earlier.
> 
> Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
> Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")

Thank you for the patch, the addition in the code looks very good to me!

But are you sure the commit you mention introduces the issue you fix here?

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

[MPTCP] Re: [PATCH net] mptcp: be careful on subflow creation

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Tue, 2020-08-04 at 21:25 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 04/08/2020 18:31, Paolo Abeni wrote:
> > Nicolas reported the following oops:
> 
> (...)
> 
> > on some unconventional configuration.
> > 
> > The MPTCP protocol is trying to create a subflow for an
> > unaccepted server socket. That is allowed by the RFC, even
> > if subflow creation will likely fail.
> > Unaccepted sockets have still a NULL sk_socket field,
> > avoid the issue by failing earlier.
> > 
> > Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski(a)tessares.net>
> > Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
> 
> Thank you for the patch, the addition in the code looks very good to me!
> 
> But are you sure the commit you mention introduces the issue you fix here?

AFAICS, the oops can be observed only with the mentioned commit - which
unconditioanlly de-reference a NULL sk->sk_socket. [try to] create a
subflow on server unaccepted socket is not a bug per-se, so I would not
send the fix to older trees.

Thanks,

Paolo

Re: [PATCH net] mptcp: be careful on subflow creation

[Paolo Abeni]

On Tue, 2020-08-04 at 21:25 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 04/08/2020 18:31, Paolo Abeni wrote:
> > Nicolas reported the following oops:
> 
> (...)
> 
> > on some unconventional configuration.
> > 
> > The MPTCP protocol is trying to create a subflow for an
> > unaccepted server socket. That is allowed by the RFC, even
> > if subflow creation will likely fail.
> > Unaccepted sockets have still a NULL sk_socket field,
> > avoid the issue by failing earlier.
> > 
> > Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
> > Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
> 
> Thank you for the patch, the addition in the code looks very good to me!
> 
> But are you sure the commit you mention introduces the issue you fix here?

AFAICS, the oops can be observed only with the mentioned commit - which
unconditioanlly de-reference a NULL sk->sk_socket. [try to] create a
subflow on server unaccepted socket is not a bug per-se, so I would not
send the fix to older trees.

Thanks,

Paolo

[MPTCP] Re: [PATCH net] mptcp: be careful on subflow creation

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

Hi Paolo,

On 05/08/2020 11:10, Paolo Abeni wrote:
> On Tue, 2020-08-04 at 21:25 +0200, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 04/08/2020 18:31, Paolo Abeni wrote:
>>> Nicolas reported the following oops:
>>
>> (...)
>>
>>> on some unconventional configuration.
>>>
>>> The MPTCP protocol is trying to create a subflow for an
>>> unaccepted server socket. That is allowed by the RFC, even
>>> if subflow creation will likely fail.
>>> Unaccepted sockets have still a NULL sk_socket field,
>>> avoid the issue by failing earlier.
>>>
>>> Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski(a)tessares.net>
>>> Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
>>
>> Thank you for the patch, the addition in the code looks very good to me!
>>
>> But are you sure the commit you mention introduces the issue you fix here?
> 
> AFAICS, the oops can be observed only with the mentioned commit - which
> unconditioanlly de-reference a NULL sk->sk_socket. [try to] create a
> subflow on server unaccepted socket is not a bug per-se, so I would not
> send the fix to older trees.

Sorry, my bad, I didn't see that in the mentioned commit, we were using 
sk->sk_socket without checking if it was not NULL...
Thank you for pointing that to me!

Bad idea to review patches on the evening :)

The patch is then good to go to me!

Reviewed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>

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

Re: [PATCH net] mptcp: be careful on subflow creation

[Matthieu Baerts]

Hi Paolo,

On 05/08/2020 11:10, Paolo Abeni wrote:
> On Tue, 2020-08-04 at 21:25 +0200, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 04/08/2020 18:31, Paolo Abeni wrote:
>>> Nicolas reported the following oops:
>>
>> (...)
>>
>>> on some unconventional configuration.
>>>
>>> The MPTCP protocol is trying to create a subflow for an
>>> unaccepted server socket. That is allowed by the RFC, even
>>> if subflow creation will likely fail.
>>> Unaccepted sockets have still a NULL sk_socket field,
>>> avoid the issue by failing earlier.
>>>
>>> Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
>>> Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
>>
>> Thank you for the patch, the addition in the code looks very good to me!
>>
>> But are you sure the commit you mention introduces the issue you fix here?
> 
> AFAICS, the oops can be observed only with the mentioned commit - which
> unconditioanlly de-reference a NULL sk->sk_socket. [try to] create a
> subflow on server unaccepted socket is not a bug per-se, so I would not
> send the fix to older trees.

Sorry, my bad, I didn't see that in the mentioned commit, we were using 
sk->sk_socket without checking if it was not NULL...
Thank you for pointing that to me!

Bad idea to review patches on the evening :)

The patch is then good to go to me!

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

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

[MPTCP] Re: [PATCH net] mptcp: be careful on subflow creation

[David Miller]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

From: Paolo Abeni <pabeni(a)redhat.com>
Date: Tue,  4 Aug 2020 18:31:06 +0200

> Nicolas reported the following oops:
 ...
> on some unconventional configuration.
> 
> The MPTCP protocol is trying to create a subflow for an
> unaccepted server socket. That is allowed by the RFC, even
> if subflow creation will likely fail.
> Unaccepted sockets have still a NULL sk_socket field,
> avoid the issue by failing earlier.
> 
> Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski(a)tessares.net>
> Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>

Applied and queued up for v5.7+ -stable, thank you.

Re: [PATCH net] mptcp: be careful on subflow creation

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue,  4 Aug 2020 18:31:06 +0200

> Nicolas reported the following oops:
 ...
> on some unconventional configuration.
> 
> The MPTCP protocol is trying to create a subflow for an
> unaccepted server socket. That is allowed by the RFC, even
> if subflow creation will likely fail.
> Unaccepted sockets have still a NULL sk_socket field,
> avoid the issue by failing earlier.
> 
> Reported-and-tested-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
> Fixes: 7d14b0d2b9b3 ("mptcp: set correct vfs info for subflows")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied and queued up for v5.7+ -stable, thank you.