1. Patchew
  2. MPTCP
  3. View series

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

Davide Caratti posted 1 patch 2 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/1207c7d44dd4d1a630afc8b7018cf4185fe4a489.1634827708.git.dcaratti@redhat.com
There is a newer version of this series
include/net/mptcp.h |  4 ++++
net/mptcp/options.c | 38 +++++++++++++++++++++++---------------
2 files changed, 27 insertions(+), 15 deletions(-)
[PATCH mptcp-net] mptcp: fix corrupt receiver key in MPC + data + checksum
Posted by Davide Caratti 2 years, 6 months ago 
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")
Link: 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>
---
 include/net/mptcp.h |  4 ++++
 net/mptcp/options.c | 38 +++++++++++++++++++++++---------------
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index f83fa48408b3..a925349b4b89 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -71,6 +71,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 422f4acfb3e6..ce27a4afc64a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -489,7 +489,7 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
 		 * 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);
@@ -1223,25 +1223,30 @@ 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 csum)
 {
 	struct csum_pseudo_header header;
-	__wsum csum;
 
 	/* cfr RFC 8684 3.3.1.:
 	 * the data sequence number used in the pseudo-header is
 	 * 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(csum));
 	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)
 {
@@ -1332,7 +1337,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;
@@ -1361,14 +1366,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
Re: [PATCH mptcp-net] mptcp: fix corrupt receiver key in MPC + data + checksum
Posted by Matthieu Baerts 2 years, 6 months ago 
Hi Davide,

On 21/10/2021 16:51, 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.

The CI found an issue with it:

Reported-by: Tessares CI <no-reply@tessares.net>

Cheers,
Matt

(PS: this includes a test for patchew)
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-net] mptcp: fix corrupt receiver key in MPC + data + checksum
Posted by Mat Martineau 2 years, 6 months ago 
On Thu, 21 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")
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/233

We generally use a "Closes:" tag for the github issues URL.

> Reported-by: Poorva Sonparote <psonparo@redhat.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> include/net/mptcp.h |  4 ++++
> net/mptcp/options.c | 38 +++++++++++++++++++++++---------------
> 2 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index f83fa48408b3..a925349b4b89 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -71,6 +71,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 422f4acfb3e6..ce27a4afc64a 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -489,7 +489,7 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
> 		 * discriminate between TCPOLEN_MPTCP_MPC_ACK_DATA and
> 		 * TCPOLEN_MPTCP_MPC_ACK
> 		 */

There's an 'ext_copy' to edit out in the first line of this comment ^
(which is just before the above context lines)

> -		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);
> @@ -1223,25 +1223,30 @@ 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 csum)
> {
> 	struct csum_pseudo_header header;
> -	__wsum csum;

As the CI automation noticed, still need a __wsum to store the return 
value of csum_partial below.

This is looking like the right way to go, with the noted few fixes.

Thanks!


>
> 	/* cfr RFC 8684 3.3.1.:
> 	 * the data sequence number used in the pseudo-header is
> 	 * 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(csum));
> 	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)
> {
> @@ -1332,7 +1337,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;
> @@ -1361,14 +1366,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
Re: [PATCH mptcp-net] mptcp: fix corrupt receiver key in MPC + data + checksum
Posted by kernel test robot 2 years, 6 months ago 
Hi Davide,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mptcp/export]
[also build test WARNING on linus/master v5.15-rc7 next-20211025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Davide-Caratti/mptcp-fix-corrupt-receiver-key-in-MPC-data-checksum/20211021-225325
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
config: x86_64-randconfig-s022-20211025 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/2293fadbec3d2e7e3c7f7da01c14f3ffa63c0e17
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Davide-Caratti/mptcp-fix-corrupt-receiver-key-in-MPC-data-checksum/20211021-225325
        git checkout 2293fadbec3d2e7e3c7f7da01c14f3ffa63c0e17
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> net/mptcp/options.c:1240:14: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __sum16 [usertype] csum @@     got restricted __wsum @@
   net/mptcp/options.c:1240:14: sparse:     expected restricted __sum16 [usertype] csum
   net/mptcp/options.c:1240:14: sparse:     got restricted __wsum
>> net/mptcp/options.c:1241:39: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected restricted __wsum [usertype] sum @@     got restricted __sum16 [usertype] csum @@
   net/mptcp/options.c:1241:39: sparse:     expected restricted __wsum [usertype] sum
   net/mptcp/options.c:1241:39: sparse:     got restricted __sum16 [usertype] csum

vim +1240 net/mptcp/options.c

  1225	
  1226	static u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __sum16 csum)
  1227	{
  1228		struct csum_pseudo_header header;
  1229	
  1230		/* cfr RFC 8684 3.3.1.:
  1231		 * the data sequence number used in the pseudo-header is
  1232		 * always the 64-bit value, irrespective of what length is used in the
  1233		 * DSS option itself.
  1234		 */
  1235		header.data_seq = cpu_to_be64(data_seq);
  1236		header.subflow_seq = htonl(subflow_seq);
  1237		header.data_len = htons(data_len);
  1238		header.csum = 0;
  1239	
> 1240		csum = csum_partial(&header, sizeof(header), ~csum_unfold(csum));
> 1241		return (__force u16)csum_fold(csum);
  1242	}
  1243	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.