| 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;
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(-)