[ovs-dev,branch-2.6] conntrack: Fix ICMPv4 error data L4 length check.
diff mbox series

Message ID 1569649640-8697-1-git-send-email-vishal.deep.ajmera@ericsson.com
State New
Headers show
Series
  • [ovs-dev,branch-2.6] conntrack: Fix ICMPv4 error data L4 length check.
Related show

Commit Message

Sriram Vatala via dev Sept. 28, 2019, 5:47 a.m. UTC
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>
(cherry picked from commit 6c2a93064afe8d812e4506880d1fd8f96108f92a)

Conflicts:
	lib/conntrack.c
---
 lib/conntrack.c | 35 ++++++++++++++++++++---------------
 lib/packets.h   |  3 +++
 2 files changed, 23 insertions(+), 15 deletions(-)

Comments

Darrell Ball Sept. 28, 2019, 6:45 p.m. UTC | #1
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
>
>
Sriram Vatala via dev Sept. 29, 2019, 10:53 a.m. UTC | #2
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

Patch
diff mbox series

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;