| Message ID | e09a16ecc0d59feb3e070e2ebf4686cf7a87c95a.1575565496.git.dcaratti@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [v4] mptcp: fix option length of mp_capable syn/ack | expand |
Looks good. Peter. On Thu, 2019-12-05 at 18:07 +0100, Davide Caratti wrote: > squashto: mptcp: Handle MPTCP TCP options > > in current MPTCP, the server sends the client's key back in the syn/ack > packet. Because of that, both tcpdump and packetdrill refuse to parse > the mp_capable option in the syn/ack. Both RFC6824 and RFC6824bis don't > require this in section 3.1, only the server's key should be sent in > the syn/ack. > > - change TCPOLEN_MPTCP_MPC_SYNACK from 20 (4+8+8) to 12 (4+8) > - don't write the client's key in mp_capable syn/ack > - adjust tests in mptcp_parse_option() to cope with the changed > value of TCPOLEN_MPTCP_MPC_SYNACK > > 3-way handshakes obtained with > > # ./mptcp_connect.sh -4 -c -e tx > [...] > # tcpdump -c3 -tnnr ns1-5de04ec0-TGs4tf-ns1-5de04ec0-TGs4tf-MPTCP-MPTCP-10.0.1.1.pcap tcp > > unpatched kernel: > > IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [S], seq 2594718977, win 65535, options [mss 65495,sackOK,TS val 3944347269 ecr 0,nop,wscale 8,mptcp capable {0xf931b304deb39a42}], length 0 > IP 10.0.1.1.10000 > 10.0.1.1.49752: Flags [S.], seq 3996234497, ack 2594718978, win 65535, options [mss 65495,sackOK,TS val 3944347270 ecr 3944347269,nop,wscale 8,mptcp capable[bad opt]> > ^^^ bad option > IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [.], ack 1, win 256, options [nop,nop,TS val 3944347270 ecr 3944347270,mptcp capable {0xf931b304deb39a42,0x96614f185e633ce}], length 0 > > patched kernel: > > IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [S], seq 626898452, win 65495, options [mss 65495,sackOK,TS val 2266878231 ecr 0,nop,wscale 7,mptcp capable {0x7fe37bfd872b9f9f}], length 0 > IP 10.0.1.1.10000 > 10.0.1.1.50850: Flags [S.], seq 3966041155, ack 626898453, win 65483, options [mss 65495,sackOK,TS val 2266878232 ecr 2266878231,nop,wscale 7,mptcp capable {0x8a167e3af39b5fc1}], length 0 > ^^^ no more bad option > IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [.], ack 1, win 512, options [nop,nop,TS val 2266878232 ecr 2266878232,mptcp capable {0x7fe37bfd872b9f9f,0x8a167e3af39b5fc1}], length 0 > > v2: more conservative behavior when parsing received options > v3: adjust tests in mptcp_parse_option() to catch all possible > values of MP_CAPABLE option length specified by v0 protocol, > thanks Matthieu Baerts > v4: avoid copying the receiver key in mptcp_synack_options(), > thanks Peter Krystad > > squashto: "mptcp: Handle MPTCP TCP options" > Fixes: 8fa520f034ed ("mptcp: Handle MPTCP TCP options") > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/mptcp/options.c | 13 +++++-------- > net/mptcp/protocol.h | 2 +- > 2 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index 98eb0281d196..b150e881ef7b 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -25,7 +25,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize, > */ > case MPTCPOPT_MP_CAPABLE: > if (opsize != TCPOLEN_MPTCP_MPC_SYN && > - opsize != TCPOLEN_MPTCP_MPC_SYNACK) > + opsize != TCPOLEN_MPTCP_MPC_ACK) > break; > > mp_opt->version = *ptr++ & MPTCP_VERSION_MASK; > @@ -57,7 +57,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize, > mp_opt->sndr_key = get_unaligned_be64(ptr); > ptr += 8; > > - if (opsize == TCPOLEN_MPTCP_MPC_SYNACK) { > + if (opsize == TCPOLEN_MPTCP_MPC_ACK) { > mp_opt->rcvr_key = get_unaligned_be64(ptr); > ptr += 8; > pr_debug("MP_CAPABLE flags=%x, sndr=%llu, rcvr=%llu", > @@ -516,11 +516,9 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size, > if (subflow_req->mp_capable) { > opts->suboptions = OPTION_MPTCP_MPC_SYNACK; > opts->sndr_key = subflow_req->local_key; > - opts->rcvr_key = subflow_req->remote_key; > *size = TCPOLEN_MPTCP_MPC_SYNACK; > - pr_debug("req=%p, local_key=%llu, remote_key=%llu", > - subflow_req, subflow_req->local_key, > - subflow_req->remote_key); > + pr_debug("req=%p, local_key=%llu", > + subflow_req, subflow_req->local_key); > return true; > } else if (subflow_req->mp_join) { > opts->suboptions = OPTION_MPTCP_MPJ_SYNACK; > @@ -650,8 +648,7 @@ void mptcp_write_options(__be32 *ptr, struct mptcp_out_options *opts) > MPTCP_CAP_HMAC_SHA1); > put_unaligned_be64(opts->sndr_key, ptr); > ptr += 2; > - if ((OPTION_MPTCP_MPC_SYNACK | > - OPTION_MPTCP_MPC_ACK) & opts->suboptions) { > + if (OPTION_MPTCP_MPC_ACK & opts->suboptions) { > put_unaligned_be64(opts->rcvr_key, ptr); > ptr += 2; > } > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index acad61c70de9..10347c9b4dff 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -34,7 +34,7 @@ > > /* MPTCP suboption lengths */ > #define TCPOLEN_MPTCP_MPC_SYN 12 > -#define TCPOLEN_MPTCP_MPC_SYNACK 20 > +#define TCPOLEN_MPTCP_MPC_SYNACK 12 > #define TCPOLEN_MPTCP_MPC_ACK 20 > #define TCPOLEN_MPTCP_MPJ_SYN 12 > #define TCPOLEN_MPTCP_MPJ_SYNACK 16
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 98eb0281d196..b150e881ef7b 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -25,7 +25,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize, */ case MPTCPOPT_MP_CAPABLE: if (opsize != TCPOLEN_MPTCP_MPC_SYN && - opsize != TCPOLEN_MPTCP_MPC_SYNACK) + opsize != TCPOLEN_MPTCP_MPC_ACK) break; mp_opt->version = *ptr++ & MPTCP_VERSION_MASK; @@ -57,7 +57,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize, mp_opt->sndr_key = get_unaligned_be64(ptr); ptr += 8; - if (opsize == TCPOLEN_MPTCP_MPC_SYNACK) { + if (opsize == TCPOLEN_MPTCP_MPC_ACK) { mp_opt->rcvr_key = get_unaligned_be64(ptr); ptr += 8; pr_debug("MP_CAPABLE flags=%x, sndr=%llu, rcvr=%llu", @@ -516,11 +516,9 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size, if (subflow_req->mp_capable) { opts->suboptions = OPTION_MPTCP_MPC_SYNACK; opts->sndr_key = subflow_req->local_key; - opts->rcvr_key = subflow_req->remote_key; *size = TCPOLEN_MPTCP_MPC_SYNACK; - pr_debug("req=%p, local_key=%llu, remote_key=%llu", - subflow_req, subflow_req->local_key, - subflow_req->remote_key); + pr_debug("req=%p, local_key=%llu", + subflow_req, subflow_req->local_key); return true; } else if (subflow_req->mp_join) { opts->suboptions = OPTION_MPTCP_MPJ_SYNACK; @@ -650,8 +648,7 @@ void mptcp_write_options(__be32 *ptr, struct mptcp_out_options *opts) MPTCP_CAP_HMAC_SHA1); put_unaligned_be64(opts->sndr_key, ptr); ptr += 2; - if ((OPTION_MPTCP_MPC_SYNACK | - OPTION_MPTCP_MPC_ACK) & opts->suboptions) { + if (OPTION_MPTCP_MPC_ACK & opts->suboptions) { put_unaligned_be64(opts->rcvr_key, ptr); ptr += 2; } diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index acad61c70de9..10347c9b4dff 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -34,7 +34,7 @@ /* MPTCP suboption lengths */ #define TCPOLEN_MPTCP_MPC_SYN 12 -#define TCPOLEN_MPTCP_MPC_SYNACK 20 +#define TCPOLEN_MPTCP_MPC_SYNACK 12 #define TCPOLEN_MPTCP_MPC_ACK 20 #define TCPOLEN_MPTCP_MPJ_SYN 12 #define TCPOLEN_MPTCP_MPJ_SYNACK 16
squashto: mptcp: Handle MPTCP TCP options in current MPTCP, the server sends the client's key back in the syn/ack packet. Because of that, both tcpdump and packetdrill refuse to parse the mp_capable option in the syn/ack. Both RFC6824 and RFC6824bis don't require this in section 3.1, only the server's key should be sent in the syn/ack. - change TCPOLEN_MPTCP_MPC_SYNACK from 20 (4+8+8) to 12 (4+8) - don't write the client's key in mp_capable syn/ack - adjust tests in mptcp_parse_option() to cope with the changed value of TCPOLEN_MPTCP_MPC_SYNACK 3-way handshakes obtained with # ./mptcp_connect.sh -4 -c -e tx [...] # tcpdump -c3 -tnnr ns1-5de04ec0-TGs4tf-ns1-5de04ec0-TGs4tf-MPTCP-MPTCP-10.0.1.1.pcap tcp unpatched kernel: IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [S], seq 2594718977, win 65535, options [mss 65495,sackOK,TS val 3944347269 ecr 0,nop,wscale 8,mptcp capable {0xf931b304deb39a42}], length 0 IP 10.0.1.1.10000 > 10.0.1.1.49752: Flags [S.], seq 3996234497, ack 2594718978, win 65535, options [mss 65495,sackOK,TS val 3944347270 ecr 3944347269,nop,wscale 8,mptcp capable[bad opt]> ^^^ bad option IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [.], ack 1, win 256, options [nop,nop,TS val 3944347270 ecr 3944347270,mptcp capable {0xf931b304deb39a42,0x96614f185e633ce}], length 0 patched kernel: IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [S], seq 626898452, win 65495, options [mss 65495,sackOK,TS val 2266878231 ecr 0,nop,wscale 7,mptcp capable {0x7fe37bfd872b9f9f}], length 0 IP 10.0.1.1.10000 > 10.0.1.1.50850: Flags [S.], seq 3966041155, ack 626898453, win 65483, options [mss 65495,sackOK,TS val 2266878232 ecr 2266878231,nop,wscale 7,mptcp capable {0x8a167e3af39b5fc1}], length 0 ^^^ no more bad option IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [.], ack 1, win 512, options [nop,nop,TS val 2266878232 ecr 2266878232,mptcp capable {0x7fe37bfd872b9f9f,0x8a167e3af39b5fc1}], length 0 v2: more conservative behavior when parsing received options v3: adjust tests in mptcp_parse_option() to catch all possible values of MP_CAPABLE option length specified by v0 protocol, thanks Matthieu Baerts v4: avoid copying the receiver key in mptcp_synack_options(), thanks Peter Krystad squashto: "mptcp: Handle MPTCP TCP options" Fixes: 8fa520f034ed ("mptcp: Handle MPTCP TCP options") Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/options.c | 13 +++++-------- net/mptcp/protocol.h | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-)