| Message ID | 1569649640-8697-1-git-send-email-vishal.deep.ajmera@ericsson.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,branch-2.6] conntrack: Fix ICMPv4 error data L4 length check. | expand |
Thanks for doing this Vishal ! Except for minor patch formatting issues (inline), this is fine and also tests fine. On Fri, Sep 27, 2019 at 10:54 PM Vishal Deep Ajmera < vishal.deep.ajmera@ericsson.com> wrote: > From: Darrell Ball <dlu998@gmail.com> > > The ICMPv4 error data L4 length check was found to be too strict for TCP, > expecting a minimum of 20 rather than 8 bytes. This worked by > hapenstance for other inner protocols. The approach is to explicitly > handle the ICMPv4 error data L4 length check and to do this for all > supported inner protocols in the same way. Making the code common > between protocols also allows the existing ICMPv4 related UDP tests to > cover TCP and ICMP inner protocol cases. > Note that ICMPv6 does not have an 8 byte limit for error L4 data. > > Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.") > CC: Daniele Di Proietto <diproiettod@ovn.org> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html > Reported-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com> > Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com> > Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com> > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > remove extra signoff when submitting the patch > (cherry picked from commit 6c2a93064afe8d812e4506880d1fd8f96108f92a) > > Conflicts: > lib/conntrack.c > move part starting from 'cherry' below the '---', although this info is not really needed > --- > lib/conntrack.c | 35 ++++++++++++++++++++--------------- > lib/packets.h | 3 +++ > 2 files changed, 23 insertions(+), 15 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 8abaf7e..d59083e 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -664,11 +664,12 @@ check_l4_icmp6(const struct conn_key *key, const > void *data, size_t size, > } > > static inline bool > -extract_l4_tcp(struct conn_key *key, const void *data, size_t size) > +extract_l4_tcp(struct conn_key *key, const void *data, size_t size, > + size_t *chk_len) > { > const struct tcp_header *tcp = data; > > - if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) { > return false; > } > > @@ -680,11 +681,12 @@ extract_l4_tcp(struct conn_key *key, const void > *data, size_t size) > } > > static inline bool > -extract_l4_udp(struct conn_key *key, const void *data, size_t size) > +extract_l4_udp(struct conn_key *key, const void *data, size_t size, > + size_t *chk_len) > { > const struct udp_header *udp = data; > > - if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) { > return false; > } > > @@ -696,7 +698,8 @@ extract_l4_udp(struct conn_key *key, const void *data, > size_t size) > } > > static inline bool extract_l4(struct conn_key *key, const void *data, > - size_t size, bool *related, const void *l3); > + size_t size, bool *related, const void *l3, > + size_t *chk_len); > > static uint8_t > reverse_icmp_type(uint8_t type) > @@ -728,11 +731,11 @@ reverse_icmp_type(uint8_t type) > * possible */ > static inline int > extract_l4_icmp(struct conn_key *key, const void *data, size_t size, > - bool *related) > + bool *related, size_t *chk_len) > { > const struct icmp_header *icmp = data; > > - if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) { > return false; > } > > @@ -783,8 +786,9 @@ extract_l4_icmp(struct conn_key *key, const void > *data, size_t size, > key->src = inner_key.src; > key->dst = inner_key.dst; > key->nw_proto = inner_key.nw_proto; > + size_t check_len = ICMP_ERROR_DATA_L4_LEN; > > - ok = extract_l4(key, l4, tail - l4, NULL, l3); > + ok = extract_l4(key, l4, tail - l4, NULL, l3, &check_len); > if (ok) { > conn_key_reverse(key); > *related = true; > @@ -872,7 +876,7 @@ extract_l4_icmp6(struct conn_key *key, const void > *data, size_t size, > key->dst = inner_key.dst; > key->nw_proto = inner_key.nw_proto; > > - ok = extract_l4(key, l4, tail - l4, NULL, l3); > + ok = extract_l4(key, l4, tail - l4, NULL, l3, NULL); > if (ok) { > conn_key_reverse(key); > *related = true; > @@ -897,21 +901,22 @@ extract_l4_icmp6(struct conn_key *key, const void > *data, size_t size, > * an ICMP or ICMP6 header. * > * > * If 'related' is NULL, it means that we're already parsing a header > nested > - * in an ICMP error. In this case, we skip checksum and length > validation. */ > + * in an ICMP error. In this case, we skip the checksum and some length > + * validations. */ > static inline bool > extract_l4(struct conn_key *key, const void *data, size_t size, bool > *related, > - const void *l3) > + const void *l3, size_t *chk_len) > { > if (key->nw_proto == IPPROTO_TCP) { > return (!related || check_l4_tcp(key, data, size, l3)) > - && extract_l4_tcp(key, data, size); > + && extract_l4_tcp(key, data, size, chk_len); > } else if (key->nw_proto == IPPROTO_UDP) { > return (!related || check_l4_udp(key, data, size, l3)) > - && extract_l4_udp(key, data, size); > + && extract_l4_udp(key, data, size, chk_len); > } else if (key->dl_type == htons(ETH_TYPE_IP) > && key->nw_proto == IPPROTO_ICMP) { > return (!related || check_l4_icmp(data, size)) > - && extract_l4_icmp(key, data, size, related); > + && extract_l4_icmp(key, data, size, related, chk_len); > } else if (key->dl_type == htons(ETH_TYPE_IPV6) > && key->nw_proto == IPPROTO_ICMPV6) { > return (!related || check_l4_icmp6(key, data, size, l3)) > @@ -982,7 +987,7 @@ conn_key_extract(struct conntrack *ct, struct > dp_packet *pkt, ovs_be16 dl_type, > > if (ok) { > if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), > - &ctx->related, l3)) { > + &ctx->related, l3, NULL)) { > ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); > return true; > } > diff --git a/lib/packets.h b/lib/packets.h > index 0705576..adfa86c 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -656,6 +656,9 @@ struct icmp_header { > }; > BUILD_ASSERT_DECL(ICMP_HEADER_LEN == sizeof(struct icmp_header)); > > +/* ICMPV4 */ > +#define ICMP_ERROR_DATA_L4_LEN 8 > + > #define IGMP_HEADER_LEN 8 > struct igmp_header { > uint8_t igmp_type; > -- > 1.9.1 > >
Hi Darrell, Ben Sent v2 patch for branch 2.6. It will also apply cleanly to branch 2.7. If looks ok, kindly merge. Warm Regards, Vishal Ajmera
diff --git a/lib/conntrack.c b/lib/conntrack.c index 8abaf7e..d59083e 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -664,11 +664,12 @@ check_l4_icmp6(const struct conn_key *key, const void *data, size_t size, } static inline bool -extract_l4_tcp(struct conn_key *key, const void *data, size_t size) +extract_l4_tcp(struct conn_key *key, const void *data, size_t size, + size_t *chk_len) { const struct tcp_header *tcp = data; - if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) { return false; } @@ -680,11 +681,12 @@ extract_l4_tcp(struct conn_key *key, const void *data, size_t size) } static inline bool -extract_l4_udp(struct conn_key *key, const void *data, size_t size) +extract_l4_udp(struct conn_key *key, const void *data, size_t size, + size_t *chk_len) { const struct udp_header *udp = data; - if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) { return false; } @@ -696,7 +698,8 @@ extract_l4_udp(struct conn_key *key, const void *data, size_t size) } static inline bool extract_l4(struct conn_key *key, const void *data, - size_t size, bool *related, const void *l3); + size_t size, bool *related, const void *l3, + size_t *chk_len); static uint8_t reverse_icmp_type(uint8_t type) @@ -728,11 +731,11 @@ reverse_icmp_type(uint8_t type) * possible */ static inline int extract_l4_icmp(struct conn_key *key, const void *data, size_t size, - bool *related) + bool *related, size_t *chk_len) { const struct icmp_header *icmp = data; - if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) { return false; } @@ -783,8 +786,9 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size, key->src = inner_key.src; key->dst = inner_key.dst; key->nw_proto = inner_key.nw_proto; + size_t check_len = ICMP_ERROR_DATA_L4_LEN; - ok = extract_l4(key, l4, tail - l4, NULL, l3); + ok = extract_l4(key, l4, tail - l4, NULL, l3, &check_len); if (ok) { conn_key_reverse(key); *related = true; @@ -872,7 +876,7 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size, key->dst = inner_key.dst; key->nw_proto = inner_key.nw_proto; - ok = extract_l4(key, l4, tail - l4, NULL, l3); + ok = extract_l4(key, l4, tail - l4, NULL, l3, NULL); if (ok) { conn_key_reverse(key); *related = true; @@ -897,21 +901,22 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size, * an ICMP or ICMP6 header. * * * If 'related' is NULL, it means that we're already parsing a header nested - * in an ICMP error. In this case, we skip checksum and length validation. */ + * in an ICMP error. In this case, we skip the checksum and some length + * validations. */ static inline bool extract_l4(struct conn_key *key, const void *data, size_t size, bool *related, - const void *l3) + const void *l3, size_t *chk_len) { if (key->nw_proto == IPPROTO_TCP) { return (!related || check_l4_tcp(key, data, size, l3)) - && extract_l4_tcp(key, data, size); + && extract_l4_tcp(key, data, size, chk_len); } else if (key->nw_proto == IPPROTO_UDP) { return (!related || check_l4_udp(key, data, size, l3)) - && extract_l4_udp(key, data, size); + && extract_l4_udp(key, data, size, chk_len); } else if (key->dl_type == htons(ETH_TYPE_IP) && key->nw_proto == IPPROTO_ICMP) { return (!related || check_l4_icmp(data, size)) - && extract_l4_icmp(key, data, size, related); + && extract_l4_icmp(key, data, size, related, chk_len); } else if (key->dl_type == htons(ETH_TYPE_IPV6) && key->nw_proto == IPPROTO_ICMPV6) { return (!related || check_l4_icmp6(key, data, size, l3)) @@ -982,7 +987,7 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type, if (ok) { if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), - &ctx->related, l3)) { + &ctx->related, l3, NULL)) { ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); return true; } diff --git a/lib/packets.h b/lib/packets.h index 0705576..adfa86c 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -656,6 +656,9 @@ struct icmp_header { }; BUILD_ASSERT_DECL(ICMP_HEADER_LEN == sizeof(struct icmp_header)); +/* ICMPV4 */ +#define ICMP_ERROR_DATA_L4_LEN 8 + #define IGMP_HEADER_LEN 8 struct igmp_header { uint8_t igmp_type;