[ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.
Darrell Ball
dlu998 at gmail.com
Mon Aug 26 16:17:06 UTC 2019
Resent this patch, as it had a bad e-mail address
Darrell
On Mon, Aug 26, 2019 at 9:06 AM Darrell Ball <dlu998 at 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 at vmware.com>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html
> Reported-by: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
> Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
> Signed-off-by: Darrell Ball <dlu998 at 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
>
>
More information about the dev
mailing list