diff mbox series

[ovs-dev,v1] ofproto-dpif-ipfix: Fix an issue in flow key part

Message ID 1518602311-16709-1-git-send-email-daniely@vmware.com
State Changes Requested
Headers show
Series [ovs-dev,v1] ofproto-dpif-ipfix: Fix an issue in flow key part | expand

Commit Message

Daniel Benli Ye Feb. 14, 2018, 9:58 a.m. UTC 
From: Benli Ye <daniely@vmware.com>

As struct ipfix_data_record_flow_key_iface didn't calculate
its length in flow key part, it may cause problem when flow
key part length is not enough. Use MAX_IF_LEN and MAX_IF_DESCR
to pre-allocate memory for ipfix_data_record_flow_key_iface.

Signed-off-by: Daniel Benli Ye <daniely@vmware.com>
---
 ofproto/ofproto-dpif-ipfix.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Ben Pfaff Feb. 14, 2018, 6:03 p.m. UTC | #1 
On Wed, Feb 14, 2018 at 01:58:31AM -0800, Daniel Benli Ye wrote:
> From: Benli Ye <daniely@vmware.com>
> 
> As struct ipfix_data_record_flow_key_iface didn't calculate
> its length in flow key part, it may cause problem when flow
> key part length is not enough. Use MAX_IF_LEN and MAX_IF_DESCR
> to pre-allocate memory for ipfix_data_record_flow_key_iface.
> 
> Signed-off-by: Daniel Benli Ye <daniely@vmware.com>

Thanks for the patch.

It looks like this fixes commit cd32509e4af4, so I'd add the following
tags to the commit message (and I'm CCing Michal):

CC: Michal Weglicki <michalx.weglicki@intel.com>
Fixes: cd32509e4af4 ("ofproto-dpif-ipfix: add interface Information Elements to flow key")


> @@ -2030,9 +2045,11 @@ ipfix_get_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
>  
>      smap_destroy(&netdev_status);
>      data->if_index = htonl(port->ifindex);
> -    data->if_descr_len = data->if_descr ? strlen(data->if_descr) : 0;
> +    data->if_descr_len = data->if_descr ? strlen(data->if_descr) > MAX_IF_DESCR_LEN
> +                             ? MAX_IF_DESCR_LEN : strlen(data->if_descr) : 0;
>      data->if_name = nullable_xstrdup(netdev_get_name(port->ofport->netdev));
> -    data->if_name_len = data->if_name ? strlen(data->if_name) : 0;
> +    data->if_name_len = data->if_name ? strlen(data->if_name) > MAX_IF_NAME_LEN
> +                            ? MAX_IF_NAME_LEN : strlen(data->if_name) : 0;

It's unusual and always a little confusing to see a ? b ? c : d : e.
I think that these can be rewritten using strnlen, e.g.:

data->if_descr ? strnlen(data->if_descr, MAX_IF_DESCR_LEN) : 0;

If you agree, would you mind posting v2?

Michal, any comments?

Thanks,

Ben.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index c7ddeb9..1058927 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -314,6 +314,7 @@  BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 20);
 /* Part of data record flow key for interface information. Since some of the
  * elements have variable length, members of this structure should be appended
  * to the 'struct dp_packet' one by one. */
+OVS_PACKED(
 struct ipfix_data_record_flow_key_iface {
     ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
     ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
@@ -321,7 +322,9 @@  struct ipfix_data_record_flow_key_iface {
     char *if_name;
     uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
     char *if_descr;
-};
+});
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_iface) ==
+                  10 + 2 * sizeof(char *));
 
 /* Part of data record flow key for VLAN entities. */
 OVS_PACKED(
@@ -511,8 +514,21 @@  BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_tcp) == 48);
  */
 #define MAX_TUNNEL_KEY_LEN 8
 
+#define MAX_IF_NAME_LEN 64
+#define MAX_IF_DESCR_LEN 128
+
+/*
+ * Calculate interface information length in flow key.
+ * This is used to calculate max flow key length.
+ */
+#define FLOW_KEY_IFACE_LEN                                      \
+    (sizeof(struct ipfix_data_record_flow_key_iface)            \
+     - 2 * sizeof(char *)                                       \
+     + MAX_IF_NAME_LEN + MAX_IF_DESCR_LEN)
+
 #define MAX_FLOW_KEY_LEN                                        \
     (sizeof(struct ipfix_data_record_flow_key_common)           \
+     + FLOW_KEY_IFACE_LEN                                       \
      + sizeof(struct ipfix_data_record_flow_key_vlan)           \
      + sizeof(struct ipfix_data_record_flow_key_ip)             \
      + MAX(sizeof(struct ipfix_data_record_flow_key_ipv4),      \
@@ -1396,7 +1412,6 @@  ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
     DEF(ETHERNET_TYPE);
     DEF(ETHERNET_HEADER_LENGTH);
 
-    /* Interface Information Elements */
     DEF(INGRESS_INTERFACE);
     DEF(INGRESS_INTERFACE_TYPE);
     DEF(INTERFACE_NAME);
@@ -2030,9 +2045,11 @@  ipfix_get_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
 
     smap_destroy(&netdev_status);
     data->if_index = htonl(port->ifindex);
-    data->if_descr_len = data->if_descr ? strlen(data->if_descr) : 0;
+    data->if_descr_len = data->if_descr ? strlen(data->if_descr) > MAX_IF_DESCR_LEN
+                             ? MAX_IF_DESCR_LEN : strlen(data->if_descr) : 0;
     data->if_name = nullable_xstrdup(netdev_get_name(port->ofport->netdev));
-    data->if_name_len = data->if_name ? strlen(data->if_name) : 0;
+    data->if_name_len = data->if_name ? strlen(data->if_name) > MAX_IF_NAME_LEN
+                            ? MAX_IF_NAME_LEN : strlen(data->if_name) : 0;
 
     return 0;
 }