[MPTCP] Re: [PATCH] mptcp: fix tcp fallback crash

[MPTCP] Re: [PATCH] mptcp: fix tcp fallback crash

[Davide Caratti]

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

On Tue, 2020-03-31 at 13:19 +0200, Florian Westphal wrote:
> Christoph Paasch reports following crash:
> 
> general protection fault, probably for non-canonical address 0xfe85e717fe88a633: 0000 [#1] PREEMPT SMP
> CPU: 0 PID: 2874 Comm: syz-executor072 Not tainted 5.6.0-rc5 #62
> RIP: 0010:__pv_queued_spin_lock_slowpath+0x1b3/0x2f0 kernel/locking/qspinlock.c:471
> [..]
>  queued_spin_lock_slowpath arch/x86/include/asm/qspinlock.h:50 [inline]
>  do_raw_spin_lock include/linux/spinlock.h:181 [inline]
>  spin_lock_bh include/linux/spinlock.h:343 [inline]
>  __mptcp_flush_join_list+0x44/0xb0 net/mptcp/protocol.c:278
>  mptcp_shutdown+0xb3/0x230 net/mptcp/protocol.c:1882
> [..]
> 
> Problem is that mptcp_shutdown() socket isn't an mptcp socket,
> its a plain tcp_sk.  Thus, trying to access mptcp_sk specific
> members accesses garbage.
> 
> Root cause is that accept() returns a fallback (tcp) socket,
> not an mptcp one.  There is code in getpeername to detect this
> and override the sockets stream_ops.  But this will only run when
> accept() caller provided a sockaddr struct.  "accept(fd, NULL, 0)"
> will therefore result in mptcp stream ops, with sock->sk being a
> tcp_sk.  Update the existing fallback handling to detect this as well.
> 
> Moreover, mptcp_shutdown did not have fallback handling, and
> mptcp_poll did it too late so add that there as well.

hi Florian,

maybe there is still a point where the NULL dereference can happen, in
mptcp_sendmsg()

 730         lock_sock(ssk);
 731         while (msg_data_left(msg)) {
 732                 ret = mptcp_sendmsg_frag(sk, ssk, msg, NULL, &timeo, &mss_now,
 733                                          &size_goal);
 734                 if (ret < 0)
 735                         break;
 736                 if (ret == 0 && unlikely(__mptcp_needs_tcp_fallback(msk))) {
 737                         release_sock(ssk);
 738                         ssock = __mptcp_tcp_fallback(msk);
 739                         goto fallback; <--- can ssock can be NULL here?
 740                 }
 741 
 742                 copied += ret;
 743         }

wdyt?

-- 
davide

[MPTCP] Re: [PATCH] mptcp: fix tcp fallback crash

[Florian Westphal]

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

Davide Caratti <dcaratti(a)redhat.com> wrote:
> On Tue, 2020-03-31 at 13:19 +0200, Florian Westphal wrote:
> > Christoph Paasch reports following crash:
> > 
> > general protection fault, probably for non-canonical address 0xfe85e717fe88a633: 0000 [#1] PREEMPT SMP
> > CPU: 0 PID: 2874 Comm: syz-executor072 Not tainted 5.6.0-rc5 #62
> > RIP: 0010:__pv_queued_spin_lock_slowpath+0x1b3/0x2f0 kernel/locking/qspinlock.c:471
> > [..]
> >  queued_spin_lock_slowpath arch/x86/include/asm/qspinlock.h:50 [inline]
> >  do_raw_spin_lock include/linux/spinlock.h:181 [inline]
> >  spin_lock_bh include/linux/spinlock.h:343 [inline]
> >  __mptcp_flush_join_list+0x44/0xb0 net/mptcp/protocol.c:278
> >  mptcp_shutdown+0xb3/0x230 net/mptcp/protocol.c:1882
> > [..]
> > 
> > Problem is that mptcp_shutdown() socket isn't an mptcp socket,
> > its a plain tcp_sk.  Thus, trying to access mptcp_sk specific
> > members accesses garbage.
> > 
> > Root cause is that accept() returns a fallback (tcp) socket,
> > not an mptcp one.  There is code in getpeername to detect this
> > and override the sockets stream_ops.  But this will only run when
> > accept() caller provided a sockaddr struct.  "accept(fd, NULL, 0)"
> > will therefore result in mptcp stream ops, with sock->sk being a
> > tcp_sk.  Update the existing fallback handling to detect this as well.
> > 
> > Moreover, mptcp_shutdown did not have fallback handling, and
> > mptcp_poll did it too late so add that there as well.
> 
> hi Florian,
> 
> maybe there is still a point where the NULL dereference can happen, in
> mptcp_sendmsg()

Not because of this specific problem.

>  730         lock_sock(ssk);
>  731         while (msg_data_left(msg)) {
>  732                 ret = mptcp_sendmsg_frag(sk, ssk, msg, NULL, &timeo, &mss_now,
>  733                                          &size_goal);
>  734                 if (ret < 0)
>  735                         break;
>  736                 if (ret == 0 && unlikely(__mptcp_needs_tcp_fallback(msk))) {
>  737                         release_sock(ssk);
>  738                         ssock = __mptcp_tcp_fallback(msk);
>  739                         goto fallback; <--- can ssock can be NULL here?
>  740                 }

It looks odd, yes.  I propose this separate patch:

Subject: mptcp: consistently retry tcp fallback lookup

There is confusion about when we can assume that the fallback socket
is non-null.  Handle this consistently and do a full re-lookup.

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -734,9 +734,9 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 			goto out;
 	}
 
+fallback:
 	ssock = __mptcp_tcp_fallback(msk);
 	if (unlikely(ssock)) {
-fallback:
 		pr_debug("fallback passthrough");
 		ret = sock_sendmsg(ssock, msg);
 		return ret >= 0 ? ret + copied : (copied ? copied : ret);
@@ -779,7 +779,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 			break;
 		if (ret == 0 && unlikely(__mptcp_needs_tcp_fallback(msk))) {
 			release_sock(ssk);
-			ssock = __mptcp_tcp_fallback(msk);
 			goto fallback;
 		}
 
@@ -889,9 +888,9 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		return -EOPNOTSUPP;
 
 	lock_sock(sk);
+fallback:
 	ssock = __mptcp_tcp_fallback(msk);
 	if (unlikely(ssock)) {
-fallback:
 		pr_debug("fallback-read subflow=%p",
 			 mptcp_subflow_ctx(ssock->sk));

[MPTCP] Re: [PATCH] mptcp: fix tcp fallback crash

[Christoph Paasch]

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

On 31/03/20 - 13:19:26, Florian Westphal wrote:
> Christoph Paasch reports following crash:
> 
> general protection fault, probably for non-canonical address 0xfe85e717fe88a633: 0000 [#1] PREEMPT SMP
> CPU: 0 PID: 2874 Comm: syz-executor072 Not tainted 5.6.0-rc5 #62
> RIP: 0010:__pv_queued_spin_lock_slowpath+0x1b3/0x2f0 kernel/locking/qspinlock.c:471
> [..]
>  queued_spin_lock_slowpath arch/x86/include/asm/qspinlock.h:50 [inline]
>  do_raw_spin_lock include/linux/spinlock.h:181 [inline]
>  spin_lock_bh include/linux/spinlock.h:343 [inline]
>  __mptcp_flush_join_list+0x44/0xb0 net/mptcp/protocol.c:278
>  mptcp_shutdown+0xb3/0x230 net/mptcp/protocol.c:1882
> [..]
> 
> Problem is that mptcp_shutdown() socket isn't an mptcp socket,
> its a plain tcp_sk.  Thus, trying to access mptcp_sk specific
> members accesses garbage.
> 
> Root cause is that accept() returns a fallback (tcp) socket,
> not an mptcp one.  There is code in getpeername to detect this
> and override the sockets stream_ops.  But this will only run when
> accept() caller provided a sockaddr struct.  "accept(fd, NULL, 0)"
> will therefore result in mptcp stream ops, with sock->sk being a
> tcp_sk.  Update the existing fallback handling to detect this as well.
> 
> Moreover, mptcp_shutdown did not have fallback handling, and
> mptcp_poll did it too late so add that there as well.
> 
> Reported-by: Christoph Paasch <cpaasch(a)apple.com>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 50 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 46 insertions(+), 4 deletions(-)

Works for me!

Tested-by: Christoph Paasch <cpaasch(a)apple.com>

I think it also needs the Fixes-tag:

Fixes: ec3edaa7ca6c ("mptcp: Add handling of outgoing MP_JOIN requests")


Christoph

[MPTCP] Re: [PATCH] mptcp: fix tcp fallback crash

[Mat Martineau]

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

On Tue, 31 Mar 2020, Florian Westphal wrote:

> Christoph Paasch reports following crash:
>
> general protection fault, probably for non-canonical address 0xfe85e717fe88a633: 0000 [#1] PREEMPT SMP
> CPU: 0 PID: 2874 Comm: syz-executor072 Not tainted 5.6.0-rc5 #62
> RIP: 0010:__pv_queued_spin_lock_slowpath+0x1b3/0x2f0 kernel/locking/qspinlock.c:471
> [..]
> queued_spin_lock_slowpath arch/x86/include/asm/qspinlock.h:50 [inline]
> do_raw_spin_lock include/linux/spinlock.h:181 [inline]
> spin_lock_bh include/linux/spinlock.h:343 [inline]
> __mptcp_flush_join_list+0x44/0xb0 net/mptcp/protocol.c:278
> mptcp_shutdown+0xb3/0x230 net/mptcp/protocol.c:1882
> [..]
>
> Problem is that mptcp_shutdown() socket isn't an mptcp socket,
> its a plain tcp_sk.  Thus, trying to access mptcp_sk specific
> members accesses garbage.
>
> Root cause is that accept() returns a fallback (tcp) socket,
> not an mptcp one.  There is code in getpeername to detect this
> and override the sockets stream_ops.  But this will only run when
> accept() caller provided a sockaddr struct.  "accept(fd, NULL, 0)"
> will therefore result in mptcp stream ops, with sock->sk being a
> tcp_sk.  Update the existing fallback handling to detect this as well.
>
> Moreover, mptcp_shutdown did not have fallback handling, and
> mptcp_poll did it too late so add that there as well.
>
> Reported-by: Christoph Paasch <cpaasch(a)apple.com>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> net/mptcp/protocol.c | 50 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 46 insertions(+), 4 deletions(-)
>

Looks good to me, thanks Florian.

Reviewed-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH] mptcp: fix tcp fallback crash

[Matthieu Baerts]

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

Hi Florian, Mat, Christoph,

On 01/04/2020 02:02, Mat Martineau wrote:
> On Tue, 31 Mar 2020, Florian Westphal wrote:
> 
>> Christoph Paasch reports following crash:
>>
>> general protection fault, probably for non-canonical address 
>> 0xfe85e717fe88a633: 0000 [#1] PREEMPT SMP
>> CPU: 0 PID: 2874 Comm: syz-executor072 Not tainted 5.6.0-rc5 #62
>> RIP: 0010:__pv_queued_spin_lock_slowpath+0x1b3/0x2f0 
>> kernel/locking/qspinlock.c:471
>> [..]
>> queued_spin_lock_slowpath arch/x86/include/asm/qspinlock.h:50 [inline]
>> do_raw_spin_lock include/linux/spinlock.h:181 [inline]
>> spin_lock_bh include/linux/spinlock.h:343 [inline]
>> __mptcp_flush_join_list+0x44/0xb0 net/mptcp/protocol.c:278
>> mptcp_shutdown+0xb3/0x230 net/mptcp/protocol.c:1882
>> [..]
>>
>> Problem is that mptcp_shutdown() socket isn't an mptcp socket,
>> its a plain tcp_sk.  Thus, trying to access mptcp_sk specific
>> members accesses garbage.
>>
>> Root cause is that accept() returns a fallback (tcp) socket,
>> not an mptcp one.  There is code in getpeername to detect this
>> and override the sockets stream_ops.  But this will only run when
>> accept() caller provided a sockaddr struct.  "accept(fd, NULL, 0)"
>> will therefore result in mptcp stream ops, with sock->sk being a
>> tcp_sk.  Update the existing fallback handling to detect this as well.
>>
>> Moreover, mptcp_shutdown did not have fallback handling, and
>> mptcp_poll did it too late so add that there as well.
>>
>> Reported-by: Christoph Paasch <cpaasch(a)apple.com>
>> Signed-off-by: Florian Westphal <fw(a)strlen.de>
>> ---
>> net/mptcp/protocol.c | 50 ++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 46 insertions(+), 4 deletions(-)
>>
> 
> Looks good to me, thanks Florian.
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>

Thank you for the patch, the review, the bug report and the test!

- 1367f91c2e4a: mptcp: fix tcp fallback crash
- 57580002e381: tg: remove accidental signed-off

I just added this patch at the end of the series (export branch)

Tests are all green on my side!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium