diff mbox series

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

Message ID 81b2a80e-2a25-4a5f-b235-07a68662aa98@randorisec.fr (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net] mptcp: Fix data stream corruption in the address announcement | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/apply fail Patch does not apply to net-0

Commit Message

Arthur Mongodin March 13, 2025, 4:26 p.m. UTC
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(-)


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

Comments

Matthieu Baerts March 13, 2025, 5:10 p.m. UTC | #1
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
Arthur Mongodin March 13, 2025, 5:18 p.m. UTC | #2
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
Matthieu Baerts March 14, 2025, 9:43 a.m. UTC | #3
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
diff mbox series

Patch

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;