[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Matthieu Baerts]

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

Hi Davide,

On 12/02/2020 10:39, Davide Caratti wrote:
> @Matthieu,
> while we are at this, what do you expect for the "official" packetdrill
> branch 'mptcp-net-next' [1]? can I do a pull request from my GH to that
> one for those tests that are currently passing (e.g. mp_capable v1, TCP
> fallback and DSN8/DACK8?)

I think you can directly work on this repo like you were doing with 
yours. At least it looks somehow "official".

Of course if you need some reviews, you can open pull requests.

You should have the write permissions. Please tell me if not.

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

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Davide Caratti]

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

On Wed, 2020-02-12 at 11:02 +0100, Matthieu Baerts wrote:
> Hi Davide,
> 
> On 12/02/2020 10:39, Davide Caratti wrote:
> > @Matthieu,
> > while we are at this, what do you expect for the "official" packetdrill
> > branch 'mptcp-net-next' [1]? can I do a pull request from my GH to that
> > one for those tests that are currently passing (e.g. mp_capable v1, TCP
> > fallback and DSN8/DACK8?)
> 
> I think you can directly work on this repo like you were doing with 
> yours. At least it looks somehow "official".

thanks! 

> Of course if you need some reviews, you can open pull requests.

... done, it's https://github.com/multipath-tcp/packetdrill/pull/1
I'm not merging it as-is, because probably DSS tests will need some make-
up after Florian's work on the DACK value - correct?

> You should have the write permissions. Please tell me if not.

I see write permissions, thanks!
-- 
davide

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Davide Caratti]

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

On Wed, 2020-02-12 at 10:39 +0100, Davide Caratti wrote:
> On Tue, 2020-02-11 at 18:42 +0100, Paolo Abeni wrote:
> > BTW I'm wondering if we are behaving correctly WRT TCP fallback for
> > passive sockets (msk->subflow = NULL): __mptcp_tcp_fallback() currently
> > returns NULL, even if the subflow has fallback to TCP. I think/fear
> > recvmsg should hang in that scenario?!? (as the server still does a
> > full bloat mptcp_recvmsg(), while the client does not provide the DSS
> > options).

from what I see, in the above case the server is not doing fallback to TCP
(see attachment). As I send to the server a packet without the DSS option,
the kernel answers with a TCP RST, that has a DSS option with DACK number
equal to the IDSN (that means probably that received data have been
discarded):

[root(a)f31 packetdrill]# cat ../mptcp/dss/fallback.pkt
`../common/defaults.sh`

+0    socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
+0    setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0    bind(3, ..., ...) = 0
+0    listen(3, 1) = 0
+0      < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7,mpcapable v1 flags[flag_h] nokey>
+0      > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8,mpcapable v1 flags[flag_h] key[skey] >
+0.01   < . 1:1(0) ack 1 win 257 <mpcapable v1 flags[flag_h] key[ckey=2,skey]>
+0    accept(3, ..., ...) = 4
+0      < . 1:101(100) ack 1 win 257
+0      > . 1:1(0) ack 101
+0    write(4, ..., 1000) = 1000
+0      > P. 1:1001(1000) ack 101
+0    close(4) = 0

[root(a)f31 packetdrill]# ./packetdrill -vvv ../mptcp/dss/fallback.pkt 
socket syscall: 1581502995.628410
setsockopt syscall: 1581502995.628771
bind syscall: 1581502995.629067
listen syscall: 1581502995.629135
inbound injected packet:  0.001306 S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7,mp_capable v1 flags: |H| >
outbound sniffed packet:  0.001768 S. 612568362:612568362(0) ack 1 win 65535 <mss 1460,nop,nop,sackOK,nop,wscale 8,mp_capable v1 flags: |H| sender_key: 16291872690117590433>
inbound injected packet:  0.013829 . 1:1(0) ack 612568363 win 257 <mp_capable v1 flags: |H| sender_key: 2 receiver_key: 16291872690117590433>
accept syscall: 1581502995.642149
inbound injected packet:  0.014330 . 1:101(100) ack 612568363 win 257 
outbound sniffed packet:  0.014460 R. 612568363:612568363(0) ack 101 win 256 <dss dack8 13263177308786788773 flags: Aa>
../mptcp/dss/fallback.pkt:13: error handling packet: live packet field ipv4_total_length: expected: 40 (0x28) vs actual: 52 (0x34)
script packet:  0.014478 . 1:1(0) ack 101 
actual packet:  0.014460 R. 1:1(0) ack 101 win 256 <dss dack8 13263177308786788773 flags: Aa>
[root(a)f31 packetdrill]# 

-- 
davide
 

[-- Attachment #2: mptcp.pcap --]
[-- Type: application/vnd.tcpdump.pcap, Size: 556 bytes --]

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Davide Caratti]

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

On Tue, 2020-02-11 at 18:42 +0100, Paolo Abeni wrote:
> BTW I'm wondering if we are behaving correctly WRT TCP fallback for
> passive sockets (msk->subflow = NULL): __mptcp_tcp_fallback() currently
> returns NULL, even if the subflow has fallback to TCP. I think/fear
> recvmsg should hang in that scenario?!? (as the server still does a
> full bloat mptcp_recvmsg(), while the client does not provide the DSS
> options).
> 
> @Davide: do we have a packetdrill to test the above? e.g. the packet
> socket is the client, it initiates an MP_CAPABLE handshake, causes
> fallback on the 3rd ack, then sends some data and (kernel) server try
> to read it with recvmsg() (possibly hanging on top of current net-next)

hi Paolo, I'm just re-building and doing a test right now. @Matthieu,
while we are at this, what do you expect for the "official" packetdrill
branch 'mptcp-net-next' [1]? can I do a pull request from my GH to that
one for those tests that are currently passing (e.g. mp_capable v1, TCP
fallback and DSN8/DACK8?)

thanks!
-- 
davide

[1] https://github.com/multipath-tcp/packetdrill

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Mat Martineau]

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


On Tue, 11 Feb 2020, Paolo Abeni wrote:

> On Mon, 2020-02-10 at 16:44 -0800, Mat Martineau wrote:
>> On Mon, 10 Feb 2020, Paolo Abeni wrote:
>>
>>> On Mon, 2020-02-10 at 12:52 +0100, Florian Westphal wrote:
>>>> Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
>>>>> Userspace should not be able to directly manipulate subflow socket
>>>>> options before a connection is established since it is not yet known if
>>>>> it will be an MPTCP subflow or a TCP fallback subflow. TCP fallback
>>>>> subflows can be more directly controlled by userspace because they are
>>>>> regular TCP connections, while MPTCP subflow sockets need to be
>>>>> configured for the specific needs of MPTCP. Use the same logic as
>>>>> sendmsg/recvmsg to ensure that socket option calls are only passed
>>>>> through to known TCP fallback subflows.
>>>>>
>>>>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>>>>> ---
>>>>>
>>>>> One other question: is it racy to have __mptcp_tcp_fallback() return an
>>>>> unlocked msk, then do the sock_hold(ssk) from an unlocked state?
>>>>
>>>> Yes.
>>>>
>>>>> It seems like if holding the ssk reference count is necessary at all it
>>>>> should be done while the msk is still locked. I'm considering a change
>>>>> to __mptcp_tcp_fallback() to have it not release the msk lock and
>>>>> instead have all the callers do that.
>>>>
>>>> Yes, that seems like the right thing to do.
>>>
>>> Agreed.
>>>
>>> I think that a separate patch for the above would be nicer.
>>>
>>
>> Thanks Florian and Paolo. It's clear what to do when handling struct sock
>> reference counts, but we are also using struct socket in two different
>> ways:
>
>>  * Calling ssock->ops->poll. The msk lock is held, but mptcp_close is
>> closing subflow sockets without the msk lock, and since we're reaching the
>> subflow without using conn_list it looks like it's possible to have the
>> subflow struct socket unexpectedly freed.
>
> [Note: I reorder the points, as the reply are simpler this way ;)]
>
> I think you are right. Likely it's enough to clear the msk->subflow
> pointer in mptcp_close() before releasing the lock. That will also
> reduce greatly the race window for the next point...

Yes, I was thinking of that. I'll make sure the combination of NULL 
msk->subflow and an empty conn_list is handled correctly.

>
>>   * Calling sock_{send,recv}msg() without the msk lock (because we don't
>> want to keep it locked while the subflow call blocks).
>
> uhm... this is quite nasty. AFAICS the possible race with the above
> mentioned change is:
>
> On CPU0 __mptcp_tcp_fallback() releases msk lock and returns the
> subflow ref
>
> On CPU1 mptcp_close() acquires the msk lock, release it, acquires the
> ssk lock, release it, free the ssk
>
> On CPU0 sock_{send,recv}msg() accesses the subflow ptr and we get UAF.
>
> So the race looks extremely unlikely, would be possible at all?
>
> Perhaps we can mark the subflows as DEAD under the msk socket lock, and
> check that on fallback? if the fallback subflow is dead we will not use
> it.
>
> BTW I'm wondering if we are behaving correctly WRT TCP fallback for
> passive sockets (msk->subflow = NULL): __mptcp_tcp_fallback() currently
> returns NULL, even if the subflow has fallback to TCP. I think/fear
> recvmsg should hang in that scenario?!? (as the server still does a
> full bloat mptcp_recvmsg(), while the client does not provide the DSS
> options).
>
> @Davide: do we have a packetdrill to test the above? e.g. the packet
> socket is the client, it initiates an MP_CAPABLE handshake, causes
> fallback on the 3rd ack, then sends some data and (kernel) server try
> to read it with recvmsg() (possibly hanging on top of current net-next)
>
> Should we resurrect the more convoluted idea of 'atomically' replacing
> struct socket->sk with msk->first delaing the deallocation of  the
> 'old' struct socket and struct sock to the ULP destroy callback?
>

We could also differentiate between "close" and "destroy", to close the 
subflow connections but make sure the socket is not deallocated until 
later and we have made sure any send/recvmsg calls have completed. Hmm, 
calling tcp_sendmsg/tcp_recvmsg would use the refcounted struct sock, 
maybe I'll try that out.

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Paolo Abeni]

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

On Mon, 2020-02-10 at 16:44 -0800, Mat Martineau wrote:
> On Mon, 10 Feb 2020, Paolo Abeni wrote:
> 
> > On Mon, 2020-02-10 at 12:52 +0100, Florian Westphal wrote:
> > > Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> > > > Userspace should not be able to directly manipulate subflow socket
> > > > options before a connection is established since it is not yet known if
> > > > it will be an MPTCP subflow or a TCP fallback subflow. TCP fallback
> > > > subflows can be more directly controlled by userspace because they are
> > > > regular TCP connections, while MPTCP subflow sockets need to be
> > > > configured for the specific needs of MPTCP. Use the same logic as
> > > > sendmsg/recvmsg to ensure that socket option calls are only passed
> > > > through to known TCP fallback subflows.
> > > > 
> > > > Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> > > > ---
> > > > 
> > > > One other question: is it racy to have __mptcp_tcp_fallback() return an
> > > > unlocked msk, then do the sock_hold(ssk) from an unlocked state?
> > > 
> > > Yes.
> > > 
> > > > It seems like if holding the ssk reference count is necessary at all it
> > > > should be done while the msk is still locked. I'm considering a change
> > > > to __mptcp_tcp_fallback() to have it not release the msk lock and
> > > > instead have all the callers do that.
> > > 
> > > Yes, that seems like the right thing to do.
> > 
> > Agreed.
> > 
> > I think that a separate patch for the above would be nicer.
> > 
> 
> Thanks Florian and Paolo. It's clear what to do when handling struct sock 
> reference counts, but we are also using struct socket in two different 
> ways:

>  * Calling ssock->ops->poll. The msk lock is held, but mptcp_close is 
> closing subflow sockets without the msk lock, and since we're reaching the 
> subflow without using conn_list it looks like it's possible to have the 
> subflow struct socket unexpectedly freed.

[Note: I reorder the points, as the reply are simpler this way ;)]

I think you are right. Likely it's enough to clear the msk->subflow
pointer in mptcp_close() before releasing the lock. That will also
reduce greatly the race window for the next point...

>   * Calling sock_{send,recv}msg() without the msk lock (because we don't 
> want to keep it locked while the subflow call blocks).

uhm... this is quite nasty. AFAICS the possible race with the above
mentioned change is:

On CPU0 __mptcp_tcp_fallback() releases msk lock and returns the
subflow ref

On CPU1 mptcp_close() acquires the msk lock, release it, acquires the
ssk lock, release it, free the ssk

On CPU0 sock_{send,recv}msg() accesses the subflow ptr and we get UAF.

So the race looks extremely unlikely, would be possible at all? 

Perhaps we can mark the subflows as DEAD under the msk socket lock, and
check that on fallback? if the fallback subflow is dead we will not use
it.

BTW I'm wondering if we are behaving correctly WRT TCP fallback for
passive sockets (msk->subflow = NULL): __mptcp_tcp_fallback() currently
returns NULL, even if the subflow has fallback to TCP. I think/fear
recvmsg should hang in that scenario?!? (as the server still does a
full bloat mptcp_recvmsg(), while the client does not provide the DSS
options).

@Davide: do we have a packetdrill to test the above? e.g. the packet
socket is the client, it initiates an MP_CAPABLE handshake, causes
fallback on the 3rd ack, then sends some data and (kernel) server try
to read it with recvmsg() (possibly hanging on top of current net-next)

Should we resurrect the more convoluted idea of 'atomically' replacing
struct socket->sk with msk->first delaing the deallocation of  the
'old' struct socket and struct sock to the ULP destroy callback?

Cheers,

Paolo

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Mat Martineau]

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

On Mon, 10 Feb 2020, Paolo Abeni wrote:

> On Mon, 2020-02-10 at 12:52 +0100, Florian Westphal wrote:
>> Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
>>> Userspace should not be able to directly manipulate subflow socket
>>> options before a connection is established since it is not yet known if
>>> it will be an MPTCP subflow or a TCP fallback subflow. TCP fallback
>>> subflows can be more directly controlled by userspace because they are
>>> regular TCP connections, while MPTCP subflow sockets need to be
>>> configured for the specific needs of MPTCP. Use the same logic as
>>> sendmsg/recvmsg to ensure that socket option calls are only passed
>>> through to known TCP fallback subflows.
>>>
>>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>>> ---
>>>
>>> One other question: is it racy to have __mptcp_tcp_fallback() return an
>>> unlocked msk, then do the sock_hold(ssk) from an unlocked state?
>>
>> Yes.
>>
>>> It seems like if holding the ssk reference count is necessary at all it
>>> should be done while the msk is still locked. I'm considering a change
>>> to __mptcp_tcp_fallback() to have it not release the msk lock and
>>> instead have all the callers do that.
>>
>> Yes, that seems like the right thing to do.
>
> Agreed.
>
> I think that a separate patch for the above would be nicer.
>

Thanks Florian and Paolo. It's clear what to do when handling struct sock 
reference counts, but we are also using struct socket in two different 
ways:

  * Calling sock_{send,recv}msg() without the msk lock (because we don't 
want to keep it locked while the subflow call blocks).

  * Calling ssock->ops->poll. The msk lock is held, but mptcp_close is 
closing subflow sockets without the msk lock, and since we're reaching the 
subflow without using conn_list it looks like it's possible to have the 
subflow struct socket unexpectedly freed.

I have a patch in progress but haven't addressed these last two issues 
yet.

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Paolo Abeni]

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

On Mon, 2020-02-10 at 12:52 +0100, Florian Westphal wrote:
> Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> > Userspace should not be able to directly manipulate subflow socket
> > options before a connection is established since it is not yet known if
> > it will be an MPTCP subflow or a TCP fallback subflow. TCP fallback
> > subflows can be more directly controlled by userspace because they are
> > regular TCP connections, while MPTCP subflow sockets need to be
> > configured for the specific needs of MPTCP. Use the same logic as
> > sendmsg/recvmsg to ensure that socket option calls are only passed
> > through to known TCP fallback subflows.
> > 
> > Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> > ---
> > 
> > One other question: is it racy to have __mptcp_tcp_fallback() return an
> > unlocked msk, then do the sock_hold(ssk) from an unlocked state?
> 
> Yes.
> 
> > It seems like if holding the ssk reference count is necessary at all it
> > should be done while the msk is still locked. I'm considering a change
> > to __mptcp_tcp_fallback() to have it not release the msk lock and
> > instead have all the callers do that.
> 
> Yes, that seems like the right thing to do.

Agreed.

I think that a separate patch for the above would be nicer.

Cheers,

Paolo

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> Userspace should not be able to directly manipulate subflow socket
> options before a connection is established since it is not yet known if
> it will be an MPTCP subflow or a TCP fallback subflow. TCP fallback
> subflows can be more directly controlled by userspace because they are
> regular TCP connections, while MPTCP subflow sockets need to be
> configured for the specific needs of MPTCP. Use the same logic as
> sendmsg/recvmsg to ensure that socket option calls are only passed
> through to known TCP fallback subflows.
> 
> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> ---
> 
> One other question: is it racy to have __mptcp_tcp_fallback() return an
> unlocked msk, then do the sock_hold(ssk) from an unlocked state?

Yes.

> It seems like if holding the ssk reference count is necessary at all it
> should be done while the msk is still locked. I'm considering a change
> to __mptcp_tcp_fallback() to have it not release the msk lock and
> instead have all the callers do that.

Yes, that seems like the right thing to do.

[MPTCP] Re: [PATCH] mptcp: Protect subflow socket options before connection completes

[Mat Martineau]

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


On Fri, 7 Feb 2020, Mat Martineau wrote:

> Userspace should not be able to directly manipulate subflow socket
> options before a connection is established since it is not yet known if
> it will be an MPTCP subflow or a TCP fallback subflow. TCP fallback
> subflows can be more directly controlled by userspace because they are
> regular TCP connections, while MPTCP subflow sockets need to be
> configured for the specific needs of MPTCP. Use the same logic as
> sendmsg/recvmsg to ensure that socket option calls are only passed
> through to known TCP fallback subflows.
>
> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> ---
>
> One other question: is it racy to have __mptcp_tcp_fallback() return an
> unlocked msk, then do the sock_hold(ssk) from an unlocked state? It
> seems like if holding the ssk reference count is necessary at all it
> should be done while the msk is still locked. I'm considering a change
> to __mptcp_tcp_fallback() to have it not release the msk lock and
> instead have all the callers do that.
>
>
> net/mptcp/protocol.c | 52 +++++++++++++++++++++++---------------------
> 1 file changed, 27 insertions(+), 25 deletions(-)


Clarification: This is a patch I'm proposing to send to netdev for the net 
repository, not something to merge to the export branch.


--
Mat Martineau
Intel