mptcp: Fix data stream corruption in the address announcement

[PATCH net] mptcp: Fix data stream corruption in the address announcement

Posted by Arthur Mongodin 3 weeks ago

The DSS and ADD_ADDR options should be exclusive and not send together.
The call to the mptcp_pm_add_addr_signal() function in the
mptcp_established_options_add_addr() function could modify opts->addr, 
thus also opts->ext_copy as they belong to distinguish entries of the 
same union field in mptcp_out_options. If the DSS option should not be 
dropped, the check if the DSS option has been previously established and 
thus if we should not establish the ADD_ADDR option is done after 
opts->addr (thus opts->ext_copy) has been modified.
This corruption may modify stream information send in the next packet
with invalid data.
Using an intermediate variable, prevents from corrupting previously
established DSS option. The assignment of the ADD_ADDR option
parameters in done once we are sure that the DSS option has been dropped
or it has not been established previously.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Fixes: 1bff1e43a30e ("mptcp: optimize out option generation")
Signed-off-by: Arthur Mongodin <amongodin@randorisec.fr>
---
  net/mptcp/options.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index fd2de185bc93..23949ae2a3a8 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -651,6 +651,7 @@ static bool 
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
  	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
  	bool drop_other_suboptions = false;
  	unsigned int opt_size = *size;
+	struct mptcp_addr_info addr;
  	bool echo;
  	int len;

@@ -659,7 +660,7 @@ static bool 
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
  	 */
  	if (!mptcp_pm_should_add_signal(msk) ||
  	    (opts->suboptions & (OPTION_MPTCP_MPJ_ACK | OPTION_MPTCP_MPC_ACK)) ||
-	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &opts->addr,
+	    !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, &addr,
  		    &echo, &drop_other_suboptions))
  		return false;

@@ -672,7 +673,7 @@ static bool 
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
  	else if (opts->suboptions & OPTION_MPTCP_DSS)
  		return false;

-	len = mptcp_add_addr_len(opts->addr.family, echo, !!opts->addr.port);
+	len = mptcp_add_addr_len(addr.family, echo, !!addr.port);
  	if (remaining < len)
  		return false;

@@ -689,6 +690,7 @@ static bool 
mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
  		opts->ahmac = 0;
  		*size -= opt_size;
  	}
+	opts->addr = addr;
  	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
  	if (!echo) {
  		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDRTX);
-- 
2.47.2

Re: [PATCH net] mptcp: Fix data stream corruption in the address announcement

Posted by MPTCP CI 3 weeks ago

Hi Arthur,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Unstable: 1 failed test(s): packetdrill_mp_join 🔴
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13840123263

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2bcfc4be9905
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=943623


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH net] mptcp: Fix data stream corruption in the address announcement

Posted by Matthieu Baerts 3 weeks ago

Hi Arthur,

Thank you for the patch.

On 13/03/2025 17:26, Arthur Mongodin wrote:
> The DSS and ADD_ADDR options should be exclusive and not send together.
> The call to the mptcp_pm_add_addr_signal() function in the
> mptcp_established_options_add_addr() function could modify opts->addr, thus also opts->ext_copy as they belong to distinguish entries of the same union field in mptcp_out_options. If the DSS option should not be dropped, the check if the DSS option has been previously established and thus if we should not establish the ADD_ADDR option is done after opts->addr (thus opts->ext_copy) has been modified.

It looks like you forgot to wrap this long line. I guess checkpatch.pl
should have complained. (Tip: 'b4' is a good handy tool to send patches)

Also, it is a bit difficult to understand this line. If that's OK, I can
update this when applying this patch to our MPTCP tree first. I will
send it back to netdev later on.

pw-bot: cr

Apart from that, the modification of the code looks good to me!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

> This corruption may modify stream information send in the next packet
> with invalid data.
> Using an intermediate variable, prevents from corrupting previously
> established DSS option. The assignment of the ADD_ADDR option
> parameters in done once we are sure that the DSS option has been dropped
> or it has not been established previously.
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Fixes: 1bff1e43a30e ("mptcp: optimize out option generation")

We will need to Cc stable as well.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH net] mptcp: Fix data stream corruption in the address announcement

Posted by Arthur Mongodin 3 weeks ago

Hi Matthieu,

On 3/13/25 18:10, Matthieu Baerts wrote:
> On 13/03/2025 17:26, Arthur Mongodin wrote:
>> The DSS and ADD_ADDR options should be exclusive and not send together.
>> The call to the mptcp_pm_add_addr_signal() function in the
>> mptcp_established_options_add_addr() function could modify opts->addr, thus also opts->ext_copy as they belong to distinguish entries of the same union field in mptcp_out_options. If the DSS option should not be dropped, the check if the DSS option has been previously established and thus if we should not establish the ADD_ADDR option is done after opts->addr (thus opts->ext_copy) has been modified.
> 
> It looks like you forgot to wrap this long line. I guess checkpatch.pl
> should have complained. (Tip: 'b4' is a good handy tool to send patches)

Sorry, I did a last minute change and I forgot to rerun
checkpatch.pl.

> Also, it is a bit difficult to understand this line. If that's OK, I can
> update this when applying this patch to our MPTCP tree first. I will
> send it back to netdev later on.

It's OK with me.

Regards,

Arthur Mongodin
Security researcher at Randorisec

Re: [PATCH net] mptcp: Fix data stream corruption in the address announcement

Posted by Matthieu Baerts 3 weeks ago

Hi Arthur, Paolo,

On 13/03/2025 18:18, Arthur Mongodin wrote:
> Hi Matthieu,
> 
> On 3/13/25 18:10, Matthieu Baerts wrote:
>> On 13/03/2025 17:26, Arthur Mongodin wrote:
>>> The DSS and ADD_ADDR options should be exclusive and not send together.
>>> The call to the mptcp_pm_add_addr_signal() function in the
>>> mptcp_established_options_add_addr() function could modify opts-
>>> >addr, thus also opts->ext_copy as they belong to distinguish entries
>>> of the same union field in mptcp_out_options. If the DSS option
>>> should not be dropped, the check if the DSS option has been
>>> previously established and thus if we should not establish the
>>> ADD_ADDR option is done after opts->addr (thus opts->ext_copy) has
>>> been modified.
>>
>> It looks like you forgot to wrap this long line. I guess checkpatch.pl
>> should have complained. (Tip: 'b4' is a good handy tool to send patches)
> 
> Sorry, I did a last minute change and I forgot to rerun
> checkpatch.pl.
> 
>> Also, it is a bit difficult to understand this line. If that's OK, I can
>> update this when applying this patch to our MPTCP tree first. I will
>> send it back to netdev later on.
> 
> It's OK with me.

@Arthur: Your patch is now in our tree -- fixes for -net -- see the
details below. I will send it to netdev later on.

@Paolo: for this fix, should I exceptionally target net-next to be part
of the next PR? Or should I target -net as usual, and we will see later
where it is best to apply it?

-------------------------------- 8< ------------------------------------
New patches for t/upstream-net and t/upstream:
- 3d5c1fa05e61: mptcp: Fix data stream corruption in the address
announcement
- Results: b9209d9d7724..125af774622b (export-net)
- Results: 9facbd5c1495..489b5d421ce3 (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/7f47a4004c06ccbde79474c4d7c2df7c8e82c739/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/e03be8fef79062df04f9f32a7ca2e4404524b5b4/checks
-------------------------------- 8< ------------------------------------

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.