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));