From: Arthur Mongodin <amongodin@randorisec.fr>

Because of the size restriction in the TCP options space, the MPTCP
ADD_ADDR option is exclusive and cannot be sent with other MPTCP ones.
For this reason, in the linked mptcp_out_options structure, group of
fields linked to different options are part of the same union.

There is a case where the mptcp_pm_add_addr_signal() function can modify
opts->addr, but not ended up sending an ADD_ADDR. Later on, back in
mptcp_established_options, other options will be sent, but with
unexpected data written in other fields due to the union, e.g. in
opts->ext_copy. This could lead to a data stream corruption in the next
packet.

Using an intermediate variable, prevents from corrupting previously
established DSS option. The assignment of the ADD_ADDR option
parameters is now done once we are sure this ADD_ADDR option can be set
in the packet, e.g. after having dropped other suboptions.

Fixes: 1bff1e43a30e ("mptcp: optimize out option generation")
Cc: stable@vger.kernel.org
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Arthur Mongodin <amongodin@randorisec.fr>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
[ Matt: the commit message has been updated: long lines splits and some
  clarifications. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 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 fd2de185bc939f8730e87a63ac02a015e610e99c..23949ae2a3a8db19d05c5c8373f45c885c3523ad 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.48.1

On Fri, Mar 14, 2025 at 09:11:31PM +0100, Matthieu Baerts (NGI0) wrote:
> From: Arthur Mongodin <amongodin@randorisec.fr>
> 
> Because of the size restriction in the TCP options space, the MPTCP
> ADD_ADDR option is exclusive and cannot be sent with other MPTCP ones.
> For this reason, in the linked mptcp_out_options structure, group of
> fields linked to different options are part of the same union.
> 
> There is a case where the mptcp_pm_add_addr_signal() function can modify
> opts->addr, but not ended up sending an ADD_ADDR. Later on, back in
> mptcp_established_options, other options will be sent, but with
> unexpected data written in other fields due to the union, e.g. in
> opts->ext_copy. This could lead to a data stream corruption in the next
> packet.
> 
> Using an intermediate variable, prevents from corrupting previously
> established DSS option. The assignment of the ADD_ADDR option
> parameters is now done once we are sure this ADD_ADDR option can be set
> in the packet, e.g. after having dropped other suboptions.
> 
> Fixes: 1bff1e43a30e ("mptcp: optimize out option generation")
> Cc: stable@vger.kernel.org
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Arthur Mongodin <amongodin@randorisec.fr>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> [ Matt: the commit message has been updated: long lines splits and some
>   clarifications. ]
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

On Fri, Mar 14, 2025 at 09:11:31PM +0100, Matthieu Baerts (NGI0) wrote:
> From: Arthur Mongodin <amongodin@randorisec.fr>
> 
> Because of the size restriction in the TCP options space, the MPTCP
> ADD_ADDR option is exclusive and cannot be sent with other MPTCP ones.
> For this reason, in the linked mptcp_out_options structure, group of
> fields linked to different options are part of the same union.
> 
> There is a case where the mptcp_pm_add_addr_signal() function can modify
> opts->addr, but not ended up sending an ADD_ADDR. Later on, back in
> mptcp_established_options, other options will be sent, but with
> unexpected data written in other fields due to the union, e.g. in
> opts->ext_copy. This could lead to a data stream corruption in the next
> packet.
> 
> Using an intermediate variable, prevents from corrupting previously
> established DSS option. The assignment of the ADD_ADDR option
> parameters is now done once we are sure this ADD_ADDR option can be set
> in the packet, e.g. after having dropped other suboptions.
> 
> Fixes: 1bff1e43a30e ("mptcp: optimize out option generation")
> Cc: stable@vger.kernel.org
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Arthur Mongodin <amongodin@randorisec.fr>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> [ Matt: the commit message has been updated: long lines splits and some
>   clarifications. ]
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>