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

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

[Davide Caratti]

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: mptcp: fix corrupt receiver key in MPC + data + checksum: Build Failure

[MPTCP CI]

Hi Davide,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1368461915

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/3ab9a5a3271a

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot

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

[Matthieu Baerts]

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

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 5147 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40989 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2985 bytes --]

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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40989 bytes --]

Re: mptcp: fix corrupt receiver key in MPC + data + checksum: Build Failure

[MPTCP CI]

Hi Davide,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1384159937

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8d20a4498bfc

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot

Re: mptcp: fix corrupt receiver key in MPC + data + checksum: Build Failure

[MPTCP CI]

Hi Davide,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/1207c7d44dd4d1a630afc8b7018cf4185fe4a489.1634827708.git.dcaratti@redhat.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1384886524

Status: failure
Initiator: matttbe
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/fc3ad97033cc

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot