Message ID | 1566835595-72476-1-git-send-email-dlu998@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,v1] conntrack: Fix ICMPV4 error data L4 length check. | expand |
Resent this patch, as it had a bad e-mail address Darrell On Mon, Aug 26, 2019 at 9:06 AM Darrell Ball <dlu998@gmail.com> wrote: > The ICMP 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 ICMP related UDP tests to > cover TCP and ICMP cases. > > Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.") > CC: Daniele Di Proietto <diproiettod@vmware.com> > 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> > --- > lib/conntrack.c | 38 ++++++++++++++++++++++---------------- > lib/packets.h | 3 +++ > 2 files changed, 25 insertions(+), 16 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index ecf3bcc..de0ab9b 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1565,9 +1565,10 @@ 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) > { > - if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) { > return false; > } > > @@ -1580,9 +1581,10 @@ 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) > { > - if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) { > return false; > } > > @@ -1596,7 +1598,7 @@ 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, > - bool validate_checksum); > + bool validate_checksum, size_t *chk_len); > > static uint8_t > reverse_icmp_type(uint8_t type) > @@ -1628,9 +1630,9 @@ 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) > { > - if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) { > return false; > } > > @@ -1681,8 +1683,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, false); > + ok = extract_l4(key, l4, tail - l4, NULL, l3, false, &check_len); > if (ok) { > conn_key_reverse(key); > *related = true; > @@ -1769,7 +1772,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, false); > + ok = extract_l4(key, l4, tail - l4, NULL, l3, false, NULL); > if (ok) { > conn_key_reverse(key); > *related = true; > @@ -1797,23 +1800,25 @@ extract_l4_icmp6(struct conn_key *key, const void > *data, size_t size, > * in an ICMP error. In this case, we skip checksum and length > validation. */ > static inline bool > extract_l4(struct conn_key *key, const void *data, size_t size, bool > *related, > - const void *l3, bool validate_checksum) > + const void *l3, bool validate_checksum, size_t *chk_len) > { > if (key->nw_proto == IPPROTO_TCP) { > return (!related || check_l4_tcp(key, data, size, l3, > - validate_checksum)) && extract_l4_tcp(key, data, size); > + validate_checksum)) > + && extract_l4_tcp(key, data, size, chk_len); > } else if (key->nw_proto == IPPROTO_UDP) { > return (!related || check_l4_udp(key, data, size, l3, > - validate_checksum)) && extract_l4_udp(key, data, size); > + validate_checksum)) > + && 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, validate_checksum)) > - && 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, > - validate_checksum)) && extract_l4_icmp6(key, data, size, > - related); > + validate_checksum)) > + && extract_l4_icmp6(key, data, size, related); > } else { > return false; > } > @@ -1892,7 +1897,8 @@ conn_key_extract(struct conntrack *ct, struct > dp_packet *pkt, ovs_be16 dl_type, > bool hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt); > /* Validate the checksum only when hwol is not supported. */ > if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), > - &ctx->icmp_related, l3, !hwol_good_l4_csum)) { > + &ctx->icmp_related, l3, !hwol_good_l4_csum, > + NULL)) { > ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); > return true; > } > diff --git a/lib/packets.h b/lib/packets.h > index 0b3e3a2..c78defb 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -780,6 +780,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 > >
diff --git a/lib/conntrack.c b/lib/conntrack.c index ecf3bcc..de0ab9b 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1565,9 +1565,10 @@ 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) { - if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) { return false; } @@ -1580,9 +1581,10 @@ 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) { - if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) { return false; } @@ -1596,7 +1598,7 @@ 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, - bool validate_checksum); + bool validate_checksum, size_t *chk_len); static uint8_t reverse_icmp_type(uint8_t type) @@ -1628,9 +1630,9 @@ 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) { - if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) { return false; } @@ -1681,8 +1683,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, false); + ok = extract_l4(key, l4, tail - l4, NULL, l3, false, &check_len); if (ok) { conn_key_reverse(key); *related = true; @@ -1769,7 +1772,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, false); + ok = extract_l4(key, l4, tail - l4, NULL, l3, false, NULL); if (ok) { conn_key_reverse(key); *related = true; @@ -1797,23 +1800,25 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size, * in an ICMP error. In this case, we skip checksum and length validation. */ static inline bool extract_l4(struct conn_key *key, const void *data, size_t size, bool *related, - const void *l3, bool validate_checksum) + const void *l3, bool validate_checksum, size_t *chk_len) { if (key->nw_proto == IPPROTO_TCP) { return (!related || check_l4_tcp(key, data, size, l3, - validate_checksum)) && extract_l4_tcp(key, data, size); + validate_checksum)) + && extract_l4_tcp(key, data, size, chk_len); } else if (key->nw_proto == IPPROTO_UDP) { return (!related || check_l4_udp(key, data, size, l3, - validate_checksum)) && extract_l4_udp(key, data, size); + validate_checksum)) + && 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, validate_checksum)) - && 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, - validate_checksum)) && extract_l4_icmp6(key, data, size, - related); + validate_checksum)) + && extract_l4_icmp6(key, data, size, related); } else { return false; } @@ -1892,7 +1897,8 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type, bool hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt); /* Validate the checksum only when hwol is not supported. */ if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), - &ctx->icmp_related, l3, !hwol_good_l4_csum)) { + &ctx->icmp_related, l3, !hwol_good_l4_csum, + NULL)) { ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); return true; } diff --git a/lib/packets.h b/lib/packets.h index 0b3e3a2..c78defb 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -780,6 +780,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;