diff mbox series

[mptcp-net,v2] mptcp: fix corrupt receiver key in MPC + data + checksum

Message ID 0ad4620ed5034e9267e579f71c060b312b3e2eee.1635258393.git.dcaratti@redhat.com (mailing list archive)
State Accepted, archived
Commit 9ed907a911bc0dfe3bc5d88ce8a2095b253ea3fc
Delegated to: Matthieu Baerts
Headers show
Series [mptcp-net,v2] mptcp: fix corrupt receiver key in MPC + data + checksum | expand

Checks

Context Check Description
matttbe/KVM_Validation__normal warning Unstable: 1 failed test(s): selftest_mptcp_join
matttbe/KVM_Validation__debug warning Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 97 lines checked
matttbe/build success Build and static analysis OK

Commit Message

Davide Caratti Oct. 26, 2021, 2:27 p.m. UTC 
using packetdrill it's possible to observe that the receiver key contains
random values when clients transmit MP_CAPABLE with data and checksum (as
specified in RFC8684 §3.1). Fix the layout of mptcp_out_options, to avoid
using the skb extension copy when writing the MP_CAPABLE sub-option.

Fixes: d7b269083786 ("mptcp: shrink mptcp_out_options struct")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/233
Reported-by: Poorva Sonparote <psonparo@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---

Notes:
    v1:
     - fix metadata in the commit message (Mat Martineau)
     - adjust comment in mptcp_established_options_mp() (Mat Martineau)
     - fix sparse warning in __mptcp_make_csum() (Mat Martineau / patchew CI)

 include/net/mptcp.h |  4 ++++
 net/mptcp/options.c | 39 ++++++++++++++++++++++++---------------
 2 files changed, 28 insertions(+), 15 deletions(-)

Comments

Mat Martineau Oct. 26, 2021, 6:50 p.m. UTC | #1 
On Tue, 26 Oct 2021, Davide Caratti wrote:

> using packetdrill it's possible to observe that the receiver key contains
> random values when clients transmit MP_CAPABLE with data and checksum (as
> specified in RFC8684 §3.1). Fix the layout of mptcp_out_options, to avoid
> using the skb extension copy when writing the MP_CAPABLE sub-option.
>
> Fixes: d7b269083786 ("mptcp: shrink mptcp_out_options struct")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/233
> Reported-by: Poorva Sonparote <psonparo@redhat.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>
> Notes:
>    v1:
>     - fix metadata in the commit message (Mat Martineau)
>     - adjust comment in mptcp_established_options_mp() (Mat Martineau)
>     - fix sparse warning in __mptcp_make_csum() (Mat Martineau / patchew CI)
>

Thanks Davide, v2 looks good and I confirmed the correct keys in MPC+data 
with 'mptcp_connect.sh -C'.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> include/net/mptcp.h |  4 ++++
> net/mptcp/options.c | 39 ++++++++++++++++++++++++---------------
> 2 files changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 6026bbefbffd..3214848402ec 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -69,6 +69,10 @@ struct mptcp_out_options {
> 		struct {
> 			u64 sndr_key;
> 			u64 rcvr_key;
> +			u64 data_seq;
> +			u32 subflow_seq;
> +			u16 data_len;
> +			__sum16 csum;
> 		};
> 		struct {
> 			struct mptcp_addr_info addr;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index c41273cefc51..f0f22eb4fd5f 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -485,11 +485,11 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
> 		mpext = mptcp_get_ext(skb);
> 		data_len = mpext ? mpext->data_len : 0;
>
> -		/* we will check ext_copy.data_len in mptcp_write_options() to
> +		/* we will check ops->data_len in mptcp_write_options() to
> 		 * discriminate between TCPOLEN_MPTCP_MPC_ACK_DATA and
> 		 * TCPOLEN_MPTCP_MPC_ACK
> 		 */
> -		opts->ext_copy.data_len = data_len;
> +		opts->data_len = data_len;
> 		opts->suboptions = OPTION_MPTCP_MPC_ACK;
> 		opts->sndr_key = subflow->local_key;
> 		opts->rcvr_key = subflow->remote_key;
> @@ -505,9 +505,9 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
> 			len = TCPOLEN_MPTCP_MPC_ACK_DATA;
> 			if (opts->csum_reqd) {
> 				/* we need to propagate more info to csum the pseudo hdr */
> -				opts->ext_copy.data_seq = mpext->data_seq;
> -				opts->ext_copy.subflow_seq = mpext->subflow_seq;
> -				opts->ext_copy.csum = mpext->csum;
> +				opts->data_seq = mpext->data_seq;
> +				opts->subflow_seq = mpext->subflow_seq;
> +				opts->csum = mpext->csum;
> 				len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> 			}
> 			*size = ALIGN(len, 4);
> @@ -1227,7 +1227,7 @@ static void mptcp_set_rwin(const struct tcp_sock *tp)
> 		WRITE_ONCE(msk->rcv_wnd_sent, ack_seq);
> }
>
> -static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> +static u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __sum16 sum)
> {
> 	struct csum_pseudo_header header;
> 	__wsum csum;
> @@ -1237,15 +1237,21 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> 	 * always the 64-bit value, irrespective of what length is used in the
> 	 * DSS option itself.
> 	 */
> -	header.data_seq = cpu_to_be64(mpext->data_seq);
> -	header.subflow_seq = htonl(mpext->subflow_seq);
> -	header.data_len = htons(mpext->data_len);
> +	header.data_seq = cpu_to_be64(data_seq);
> +	header.subflow_seq = htonl(subflow_seq);
> +	header.data_len = htons(data_len);
> 	header.csum = 0;
>
> -	csum = csum_partial(&header, sizeof(header), ~csum_unfold(mpext->csum));
> +	csum = csum_partial(&header, sizeof(header), ~csum_unfold(sum));
> 	return (__force u16)csum_fold(csum);
> }
>
> +static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> +{
> +	return __mptcp_make_csum(mpext->data_seq, mpext->subflow_seq, mpext->data_len,
> +				 mpext->csum);
> +}
> +
> void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 			 struct mptcp_out_options *opts)
> {
> @@ -1337,7 +1343,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 			len = TCPOLEN_MPTCP_MPC_SYN;
> 		} else if (OPTION_MPTCP_MPC_SYNACK & opts->suboptions) {
> 			len = TCPOLEN_MPTCP_MPC_SYNACK;
> -		} else if (opts->ext_copy.data_len) {
> +		} else if (opts->data_len) {
> 			len = TCPOLEN_MPTCP_MPC_ACK_DATA;
> 			if (opts->csum_reqd)
> 				len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> @@ -1366,14 +1372,17 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>
> 		put_unaligned_be64(opts->rcvr_key, ptr);
> 		ptr += 2;
> -		if (!opts->ext_copy.data_len)
> +		if (!opts->data_len)
> 			goto mp_capable_done;
>
> 		if (opts->csum_reqd) {
> -			put_unaligned_be32(opts->ext_copy.data_len << 16 |
> -					   mptcp_make_csum(&opts->ext_copy), ptr);
> +			put_unaligned_be32(opts->data_len << 16 |
> +					   __mptcp_make_csum(opts->data_seq,
> +							     opts->subflow_seq,
> +							     opts->data_len,
> +							     opts->csum), ptr);
> 		} else {
> -			put_unaligned_be32(opts->ext_copy.data_len << 16 |
> +			put_unaligned_be32(opts->data_len << 16 |
> 					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> 		}
> 		ptr += 1;
> -- 
> 2.31.1
>
>

--
Mat Martineau
Intel
Mat Martineau Oct. 26, 2021, 11:10 p.m. UTC | #2 
On Tue, 26 Oct 2021, Mat Martineau wrote:

> On Tue, 26 Oct 2021, Davide Caratti wrote:
>
>> using packetdrill it's possible to observe that the receiver key contains
>> random values when clients transmit MP_CAPABLE with data and checksum (as
>> specified in RFC8684 §3.1). Fix the layout of mptcp_out_options, to avoid
>> using the skb extension copy when writing the MP_CAPABLE sub-option.
>> 
>> Fixes: d7b269083786 ("mptcp: shrink mptcp_out_options struct")
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/233
>> Reported-by: Poorva Sonparote <psonparo@redhat.com>
>> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
>> ---
>> 
>> Notes:
>>    v1:
>>     - fix metadata in the commit message (Mat Martineau)
>>     - adjust comment in mptcp_established_options_mp() (Mat Martineau)
>>     - fix sparse warning in __mptcp_make_csum() (Mat Martineau / patchew 
>> CI)
>> 
>
> Thanks Davide, v2 looks good and I confirmed the correct keys in MPC+data 
> with 'mptcp_connect.sh -C'.
>
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>

Matthieu -

One more thing: If your schedule allows, could you apply this early 
Wednesday? That way CI can complete, and then there's a chance to sync up 
on IRC when our timezones overlap. Hopefully I can send this to netdev in 
the morning (my time), probably the last chance to get it merged for 
v5.15.

--
Mat Martineau
Intel
Matthieu Baerts Oct. 27, 2021, 7:06 p.m. UTC | #3 
Hi Davide, Mat,

On 26/10/2021 16:27, Davide Caratti wrote:
> using packetdrill it's possible to observe that the receiver key contains
> random values when clients transmit MP_CAPABLE with data and checksum (as
> specified in RFC8684 §3.1). Fix the layout of mptcp_out_options, to avoid
> using the skb extension copy when writing the MP_CAPABLE sub-option.

Thank you for the patch and the review (sorry, I missed Mat's replies...)

Now in our tree (fixes for -net):

- 9ed907a911bc: mptcp: fix corrupt receiver key in MPC + data + checksum
- Results: 379d8f879e9a..0c31fd9a8970

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20211027T190543
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

Cheers,
Matt
diff mbox series

Patch

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 6026bbefbffd..3214848402ec 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -69,6 +69,10 @@  struct mptcp_out_options {
 		struct {
 			u64 sndr_key;
 			u64 rcvr_key;
+			u64 data_seq;
+			u32 subflow_seq;
+			u16 data_len;
+			__sum16 csum;
 		};
 		struct {
 			struct mptcp_addr_info addr;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c41273cefc51..f0f22eb4fd5f 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -485,11 +485,11 @@  static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
 		mpext = mptcp_get_ext(skb);
 		data_len = mpext ? mpext->data_len : 0;
 
-		/* we will check ext_copy.data_len in mptcp_write_options() to
+		/* we will check ops->data_len in mptcp_write_options() to
 		 * discriminate between TCPOLEN_MPTCP_MPC_ACK_DATA and
 		 * TCPOLEN_MPTCP_MPC_ACK
 		 */
-		opts->ext_copy.data_len = data_len;
+		opts->data_len = data_len;
 		opts->suboptions = OPTION_MPTCP_MPC_ACK;
 		opts->sndr_key = subflow->local_key;
 		opts->rcvr_key = subflow->remote_key;
@@ -505,9 +505,9 @@  static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
 			len = TCPOLEN_MPTCP_MPC_ACK_DATA;
 			if (opts->csum_reqd) {
 				/* we need to propagate more info to csum the pseudo hdr */
-				opts->ext_copy.data_seq = mpext->data_seq;
-				opts->ext_copy.subflow_seq = mpext->subflow_seq;
-				opts->ext_copy.csum = mpext->csum;
+				opts->data_seq = mpext->data_seq;
+				opts->subflow_seq = mpext->subflow_seq;
+				opts->csum = mpext->csum;
 				len += TCPOLEN_MPTCP_DSS_CHECKSUM;
 			}
 			*size = ALIGN(len, 4);
@@ -1227,7 +1227,7 @@  static void mptcp_set_rwin(const struct tcp_sock *tp)
 		WRITE_ONCE(msk->rcv_wnd_sent, ack_seq);
 }
 
-static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
+static u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __sum16 sum)
 {
 	struct csum_pseudo_header header;
 	__wsum csum;
@@ -1237,15 +1237,21 @@  static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
 	 * always the 64-bit value, irrespective of what length is used in the
 	 * DSS option itself.
 	 */
-	header.data_seq = cpu_to_be64(mpext->data_seq);
-	header.subflow_seq = htonl(mpext->subflow_seq);
-	header.data_len = htons(mpext->data_len);
+	header.data_seq = cpu_to_be64(data_seq);
+	header.subflow_seq = htonl(subflow_seq);
+	header.data_len = htons(data_len);
 	header.csum = 0;
 
-	csum = csum_partial(&header, sizeof(header), ~csum_unfold(mpext->csum));
+	csum = csum_partial(&header, sizeof(header), ~csum_unfold(sum));
 	return (__force u16)csum_fold(csum);
 }
 
+static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
+{
+	return __mptcp_make_csum(mpext->data_seq, mpext->subflow_seq, mpext->data_len,
+				 mpext->csum);
+}
+
 void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			 struct mptcp_out_options *opts)
 {
@@ -1337,7 +1343,7 @@  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			len = TCPOLEN_MPTCP_MPC_SYN;
 		} else if (OPTION_MPTCP_MPC_SYNACK & opts->suboptions) {
 			len = TCPOLEN_MPTCP_MPC_SYNACK;
-		} else if (opts->ext_copy.data_len) {
+		} else if (opts->data_len) {
 			len = TCPOLEN_MPTCP_MPC_ACK_DATA;
 			if (opts->csum_reqd)
 				len += TCPOLEN_MPTCP_DSS_CHECKSUM;
@@ -1366,14 +1372,17 @@  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 
 		put_unaligned_be64(opts->rcvr_key, ptr);
 		ptr += 2;
-		if (!opts->ext_copy.data_len)
+		if (!opts->data_len)
 			goto mp_capable_done;
 
 		if (opts->csum_reqd) {
-			put_unaligned_be32(opts->ext_copy.data_len << 16 |
-					   mptcp_make_csum(&opts->ext_copy), ptr);
+			put_unaligned_be32(opts->data_len << 16 |
+					   __mptcp_make_csum(opts->data_seq,
+							     opts->subflow_seq,
+							     opts->data_len,
+							     opts->csum), ptr);
 		} else {
-			put_unaligned_be32(opts->ext_copy.data_len << 16 |
+			put_unaligned_be32(opts->data_len << 16 |
 					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
 		}
 		ptr += 1;