| Message ID | 81b2a80e-2a25-4a5f-b235-07a68662aa98@randorisec.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3d5c1fa05e6121d68d2dcc84658718c2e982fb25 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [net] mptcp: Fix data stream corruption in the address announcement | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 30 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | warning | Unstable: 1 failed test(s): packetdrill_mp_join |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | success | Success! ✅ |
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
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
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
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 --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;
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);