mptcp: remove tcp ulp setsockopt support

[PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Florian Westphal 4 years, 2 months ago

TCP_ULP setsockopt cannot be used for mptcp because its already
used internally to plumb subflow (tcp) sockets to the mptcp layer.

syzbot managed to trigger a crash for mptcp connections that are
in fallback mode:

KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
CPU: 1 PID: 1083 Comm: syz-executor.3 Not tainted 5.16.0-rc2-syzkaller #0
RIP: 0010:tls_build_proto net/tls/tls_main.c:776 [inline]
[..]
 __tcp_set_ulp net/ipv4/tcp_ulp.c:139 [inline]
 tcp_set_ulp+0x428/0x4c0 net/ipv4/tcp_ulp.c:160
 do_tcp_setsockopt+0x455/0x37c0 net/ipv4/tcp.c:3391
 mptcp_setsockopt+0x1b47/0x2400 net/mptcp/sockopt.c:638

Remove support for TCP_ULP setsockopt.

Reported-by: syzbot+1fd9b69cde42967d1add@syzkaller.appspotmail.com
Signed-off-by: Florian Westphal <fw@strlen.de>
---
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 3c3db22fd36a..aa3fcd86dbe2 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -543,7 +543,6 @@ static bool mptcp_supported_sockopt(int level, int optname)
 		case TCP_NODELAY:
 		case TCP_THIN_LINEAR_TIMEOUTS:
 		case TCP_CONGESTION:
-		case TCP_ULP:
 		case TCP_CORK:
 		case TCP_KEEPIDLE:
 		case TCP_KEEPINTVL:
-- 
2.32.0

Re: [PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Matthieu Baerts 4 years, 2 months ago

Hi Florian,

(without netdev and syzbot ML)

On 05/12/2021 20:27, Florian Westphal wrote:
> TCP_ULP setsockopt cannot be used for mptcp because its already
> used internally to plumb subflow (tcp) sockets to the mptcp layer.
> 
> syzbot managed to trigger a crash for mptcp connections that are
> in fallback mode:
> 
> KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
> CPU: 1 PID: 1083 Comm: syz-executor.3 Not tainted 5.16.0-rc2-syzkaller #0
> RIP: 0010:tls_build_proto net/tls/tls_main.c:776 [inline]
> [..]
>  __tcp_set_ulp net/ipv4/tcp_ulp.c:139 [inline]
>  tcp_set_ulp+0x428/0x4c0 net/ipv4/tcp_ulp.c:160
>  do_tcp_setsockopt+0x455/0x37c0 net/ipv4/tcp.c:3391
>  mptcp_setsockopt+0x1b47/0x2400 net/mptcp/sockopt.c:638
> 
> Remove support for TCP_ULP setsockopt.

Good catch!
Indeed, it doesn't make sense to support TCP_ULP.

> Reported-by: syzbot+1fd9b69cde42967d1add@syzkaller.appspotmail.com
> Signed-off-by: Florian Westphal <fw@strlen.de>

I guess we need a Fixes tag here:

Fixes: d9e4c1291810 ("mptcp: only admit explicitly supported sockopt")

BTW, did you send this patch to netdev ML on purpose?
I guess this is fine but they will probably ask for the Fixes tag as it
looks like a bug-fix. Do you want me to reply to this email on netdev
and include this "Fixes" tag in my email?

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

Re: [PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Florian Westphal 4 years, 2 months ago

Matthieu Baerts <matthieu.baerts@tessares.net> wrote:
> BTW, did you send this patch to netdev ML on purpose?

Yes, it was a reply to syzbot report.

> I guess this is fine but they will probably ask for the Fixes tag as it
> looks like a bug-fix. Do you want me to reply to this email on netdev
> and include this "Fixes" tag in my email?

I think it can go in via mptcp tree.
This patch is fine but there is still another ULP bug left, will send a
patch soon.

Re: [PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Mat Martineau 4 years, 2 months ago

On Mon, 6 Dec 2021, Matthieu Baerts wrote:

> Hi Florian,
>
> (without netdev and syzbot ML)
>
> On 05/12/2021 20:27, Florian Westphal wrote:
>> TCP_ULP setsockopt cannot be used for mptcp because its already
>> used internally to plumb subflow (tcp) sockets to the mptcp layer.
>>
>> syzbot managed to trigger a crash for mptcp connections that are
>> in fallback mode:
>>
>> KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
>> CPU: 1 PID: 1083 Comm: syz-executor.3 Not tainted 5.16.0-rc2-syzkaller #0
>> RIP: 0010:tls_build_proto net/tls/tls_main.c:776 [inline]
>> [..]
>>  __tcp_set_ulp net/ipv4/tcp_ulp.c:139 [inline]
>>  tcp_set_ulp+0x428/0x4c0 net/ipv4/tcp_ulp.c:160
>>  do_tcp_setsockopt+0x455/0x37c0 net/ipv4/tcp.c:3391
>>  mptcp_setsockopt+0x1b47/0x2400 net/mptcp/sockopt.c:638
>>
>> Remove support for TCP_ULP setsockopt.
>
> Good catch!
> Indeed, it doesn't make sense to support TCP_ULP.
>
>> Reported-by: syzbot+1fd9b69cde42967d1add@syzkaller.appspotmail.com
>> Signed-off-by: Florian Westphal <fw@strlen.de>
>
> I guess we need a Fixes tag here:
>
> Fixes: d9e4c1291810 ("mptcp: only admit explicitly supported sockopt")
>

Thanks Florian.

Matthieu, could you apply this to the export branch with your suggested 
Fixes tag?

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

--
Mat Martineau
Intel

Re: [PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Matthieu Baerts 4 years, 2 months ago

Hi Mat,

On 06/12/2021 19:47, Mat Martineau wrote:
> On Mon, 6 Dec 2021, Matthieu Baerts wrote:
> 
>> Hi Florian,
>>
>> (without netdev and syzbot ML)
>>
>> On 05/12/2021 20:27, Florian Westphal wrote:
>>> TCP_ULP setsockopt cannot be used for mptcp because its already
>>> used internally to plumb subflow (tcp) sockets to the mptcp layer.
>>>
>>> syzbot managed to trigger a crash for mptcp connections that are
>>> in fallback mode:
>>>
>>> KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
>>> CPU: 1 PID: 1083 Comm: syz-executor.3 Not tainted
>>> 5.16.0-rc2-syzkaller #0
>>> RIP: 0010:tls_build_proto net/tls/tls_main.c:776 [inline]
>>> [..]
>>>  __tcp_set_ulp net/ipv4/tcp_ulp.c:139 [inline]
>>>  tcp_set_ulp+0x428/0x4c0 net/ipv4/tcp_ulp.c:160
>>>  do_tcp_setsockopt+0x455/0x37c0 net/ipv4/tcp.c:3391
>>>  mptcp_setsockopt+0x1b47/0x2400 net/mptcp/sockopt.c:638
>>>
>>> Remove support for TCP_ULP setsockopt.
>>
>> Good catch!
>> Indeed, it doesn't make sense to support TCP_ULP.
>>
>>> Reported-by: syzbot+1fd9b69cde42967d1add@syzkaller.appspotmail.com
>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>
>> I guess we need a Fixes tag here:
>>
>> Fixes: d9e4c1291810 ("mptcp: only admit explicitly supported sockopt")
>>
> 
> Thanks Florian.
> 
> Matthieu, could you apply this to the export branch with your suggested
> Fixes tag?
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Sure, just did:

- 13dc812544f7: mptcp: remove tcp ulp setsockopt support

- Results: 5fd3630ba52a..4be3d8d5b45c

Builds and tests are now in progress:



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

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 mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Jakub Kicinski 4 years, 2 months ago

On Sun,  5 Dec 2021 20:27:00 +0100 Florian Westphal wrote:
> TCP_ULP setsockopt cannot be used for mptcp because its already
> used internally to plumb subflow (tcp) sockets to the mptcp layer.
> 
> syzbot managed to trigger a crash for mptcp connections that are
> in fallback mode:

Fallback mode meaning ops are NULL? I'm slightly confused by this
report.

> KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
> CPU: 1 PID: 1083 Comm: syz-executor.3 Not tainted 5.16.0-rc2-syzkaller #0
> RIP: 0010:tls_build_proto net/tls/tls_main.c:776 [inline]
> [..]
>  __tcp_set_ulp net/ipv4/tcp_ulp.c:139 [inline]
>  tcp_set_ulp+0x428/0x4c0 net/ipv4/tcp_ulp.c:160
>  do_tcp_setsockopt+0x455/0x37c0 net/ipv4/tcp.c:3391
>  mptcp_setsockopt+0x1b47/0x2400 net/mptcp/sockopt.c:638
> 
> Remove support for TCP_ULP setsockopt.
> 
> Reported-by: syzbot+1fd9b69cde42967d1add@syzkaller.appspotmail.com
> Signed-off-by: Florian Westphal <fw@strlen.de>

Re: [PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Jakub Kicinski 4 years, 2 months ago

On Mon, 6 Dec 2021 07:53:26 -0800 Jakub Kicinski wrote:
> On Sun,  5 Dec 2021 20:27:00 +0100 Florian Westphal wrote:
> > TCP_ULP setsockopt cannot be used for mptcp because its already
> > used internally to plumb subflow (tcp) sockets to the mptcp layer.
> > 
> > syzbot managed to trigger a crash for mptcp connections that are
> > in fallback mode:  
> 
> Fallback mode meaning ops are NULL? I'm slightly confused by this
> report.

Ah, it's the socket not the ops.

> > KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
> > CPU: 1 PID: 1083 Comm: syz-executor.3 Not tainted 5.16.0-rc2-syzkaller #0
> > RIP: 0010:tls_build_proto net/tls/tls_main.c:776 [inline]
> > [..]
> >  __tcp_set_ulp net/ipv4/tcp_ulp.c:139 [inline]
> >  tcp_set_ulp+0x428/0x4c0 net/ipv4/tcp_ulp.c:160
> >  do_tcp_setsockopt+0x455/0x37c0 net/ipv4/tcp.c:3391
> >  mptcp_setsockopt+0x1b47/0x2400 net/mptcp/sockopt.c:638
> > 
> > Remove support for TCP_ULP setsockopt.
> > 
> > Reported-by: syzbot+1fd9b69cde42967d1add@syzkaller.appspotmail.com
> > Signed-off-by: Florian Westphal <fw@strlen.de>

Re: [PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Mat Martineau 4 years, 2 months ago

On Mon, 6 Dec 2021, Jakub Kicinski wrote:

> On Mon, 6 Dec 2021 07:53:26 -0800 Jakub Kicinski wrote:
>> On Sun,  5 Dec 2021 20:27:00 +0100 Florian Westphal wrote:
>>> TCP_ULP setsockopt cannot be used for mptcp because its already
>>> used internally to plumb subflow (tcp) sockets to the mptcp layer.
>>>
>>> syzbot managed to trigger a crash for mptcp connections that are
>>> in fallback mode:
>>
>> Fallback mode meaning ops are NULL? I'm slightly confused by this
>> report.
>
> Ah, it's the socket not the ops.
>
>>> KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
>>> CPU: 1 PID: 1083 Comm: syz-executor.3 Not tainted 5.16.0-rc2-syzkaller #0
>>> RIP: 0010:tls_build_proto net/tls/tls_main.c:776 [inline]
>>> [..]
>>>  __tcp_set_ulp net/ipv4/tcp_ulp.c:139 [inline]
>>>  tcp_set_ulp+0x428/0x4c0 net/ipv4/tcp_ulp.c:160
>>>  do_tcp_setsockopt+0x455/0x37c0 net/ipv4/tcp.c:3391
>>>  mptcp_setsockopt+0x1b47/0x2400 net/mptcp/sockopt.c:638
>>>
>>> Remove support for TCP_ULP setsockopt.
>>>
>>> Reported-by: syzbot+1fd9b69cde42967d1add@syzkaller.appspotmail.com
>>> Signed-off-by: Florian Westphal <fw@strlen.de>

Jakub -

If you could mark this as "Not Applicable" in netdevbpf patchwork, we'll 
apply it to the mptcp tree and resubmit to netdev with some related 
patches.

Thanks,

--
Mat Martineau
Intel

Re: [PATCH mptcp] mptcp: remove tcp ulp setsockopt support

Posted by Jakub Kicinski 4 years, 2 months ago

On Mon, 6 Dec 2021 10:44:16 -0800 (PST) Mat Martineau wrote:
> If you could mark this as "Not Applicable" in netdevbpf patchwork, we'll 
> apply it to the mptcp tree and resubmit to netdev with some related 
> patches.

For sure, I was asking because technically I added the code that hit
the oops to tls a week or so ago, so I wanted to make sure it's not 
my fault ;) Marked in pw appropriately now.