| Message ID | 1207c7d44dd4d1a630afc8b7018cf4185fe4a489.1634827708.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [mptcp-net] mptcp: fix corrupt receiver key in MPC + data + checksum | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 95 lines checked |
| matttbe/build | warning | Build error with: make C=1 net/mptcp/options.o |
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
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)
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
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
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
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
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;
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(-)