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

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.

v1 -> v2: Use strnlen() to limit the max length.

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

On Wed, Feb 14, 2018 at 05:52:07PM -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.
> 
> v1 -> v2: Use strnlen() to limit the max length.
> 
> Signed-off-by: Daniel Benli Ye <daniely@vmware.com>

Looks good to me.  Thank you!  I'll give Michal a day or so to comment.

Hi Daniel, Ben, 

Looks good to me!

Br, 
Michal. 

> -----Original Message-----
> From: Daniel Benli Ye [mailto:daniely@vmware.com]
> Sent: Thursday, February 15, 2018 2:52 AM
> To: blp@ovn.com; wenyuz@vmware.com; Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org
> Cc: Benli Ye <daniely@vmware.com>
> Subject: [PATCH v2] ofproto-dpif-ipfix: Fix an issue in flow key part
> 
> 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.
> 
> v1 -> v2: Use strnlen() to limit the max length.
> 
> Signed-off-by: Daniel Benli Ye <daniely@vmware.com>
> ---
>  ofproto/ofproto-dpif-ipfix.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index c7ddeb9..4d9fe78 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),      \
> @@ -2030,9 +2046,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 ? strnlen(data->if_descr,
> +                                                  MAX_IF_DESCR_LEN) : 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 ? strnlen(data->if_name,
> +                                                MAX_IF_NAME_LEN) : 0;
> 
>      return 0;
>  }
> --
> 2.7.4

Thanks Daniel and Michal.  I applied this to master and branch-2.9.

On Thu, Feb 15, 2018 at 01:23:04PM +0000, Weglicki, MichalX wrote:
> Hi Daniel, Ben, 
> 
> Looks good to me!
> 
> Br, 
> Michal. 
> 
> > -----Original Message-----
> > From: Daniel Benli Ye [mailto:daniely@vmware.com]
> > Sent: Thursday, February 15, 2018 2:52 AM
> > To: blp@ovn.com; wenyuz@vmware.com; Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org
> > Cc: Benli Ye <daniely@vmware.com>
> > Subject: [PATCH v2] ofproto-dpif-ipfix: Fix an issue in flow key part
> > 
> > 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.
> > 
> > v1 -> v2: Use strnlen() to limit the max length.
> > 
> > Signed-off-by: Daniel Benli Ye <daniely@vmware.com>
> > ---
> >  ofproto/ofproto-dpif-ipfix.c | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index c7ddeb9..4d9fe78 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),      \
> > @@ -2030,9 +2046,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 ? strnlen(data->if_descr,
> > +                                                  MAX_IF_DESCR_LEN) : 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 ? strnlen(data->if_name,
> > +                                                MAX_IF_NAME_LEN) : 0;
> > 
> >      return 0;
> >  }
> > --
> > 2.7.4
>