[v2,1/4] net: new ipv6 header extension parsing function

  Introduce new function for IPv6 header extension parsing able to
determine extension length and next protocol number.

This function is helpful when implementing IPv6 header traversing.

Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
---
 lib/librte_net/rte_ip.h | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
  

> -----Original Message-----
> From: Smoczynski, MarcinX
> Sent: Monday, June 24, 2019 2:40 PM
> To: Kovacevic, Marko <marko.kovacevic@intel.com>; orika@mellanox.com; Richardson, Bruce <bruce.richardson@intel.com>; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>; akhil.goyal@nxp.com; Kantecki, Tomasz
> <tomasz.kantecki@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; olivier.matz@6wind.com
> Cc: dev@dpdk.org; Smoczynski, MarcinX <marcinx.smoczynski@intel.com>
> Subject: [PATCH v2 1/4] net: new ipv6 header extension parsing function
> 
> Introduce new function for IPv6 header extension parsing able to
> determine extension length and next protocol number.
> 
> This function is helpful when implementing IPv6 header traversing.
> 
> Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> ---
>  lib/librte_net/rte_ip.h | 49 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index ae3b7e730..c2c67b85d 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -428,6 +428,55 @@ rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr)
>  	return (uint16_t)cksum;
>  }
> 
> +/* IPv6 fragmentation header size */
> +#define RTE_IPV6_FRAG_HDR_SIZE 8
> +
> +/**
> + * Parse next IPv6 header extension
> + *
> + * This function checks if proto number is an IPv6 extensions and parses its
> + * data if so, providing information on next header and extension length.
> + *
> + * @param p
> + *   Pointer to an extension raw data.
> + * @param proto
> + *   Protocol number extracted from the "next header" field from
> + *   the IPv6 header or the previous extension.
> + * @param ext_len
> + *   Extension data length.
> + * @return
> + *   next protocol number if proto is an IPv6 extension, -EINVAL otherwise
> + */
> +static inline int __rte_experimental
> +rte_ipv6_get_next_ext(uint8_t *p, int proto, size_t *ext_len)
> +{
> +	int next_proto;
> +
> +	switch (proto) {
> +	case IPPROTO_AH:
> +		next_proto = *p++;
> +		*ext_len = (*p + 2) * sizeof(uint32_t);
> +		break;
> +
> +	case IPPROTO_HOPOPTS:
> +	case IPPROTO_ROUTING:
> +	case IPPROTO_DSTOPTS:
> +		next_proto = *p++;
> +		*ext_len = (*p + 1) * sizeof(uint64_t);
> +		break;
> +
> +	case IPPROTO_FRAGMENT:
> +		next_proto = *p;
> +		*ext_len = RTE_IPV6_FRAG_HDR_SIZE;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return next_proto;
> +}
> +
>  #ifdef __cplusplus
>  }
>  #endif
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Tested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1
  

Hi,

On Mon, Jun 24, 2019 at 03:39:57PM +0200, Marcin Smoczynski wrote:
> Introduce new function for IPv6 header extension parsing able to
> determine extension length and next protocol number.
> 
> This function is helpful when implementing IPv6 header traversing.
> 
> Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> ---
>  lib/librte_net/rte_ip.h | 49 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index ae3b7e730..c2c67b85d 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -428,6 +428,55 @@ rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr)
>  	return (uint16_t)cksum;
>  }
>  
> +/* IPv6 fragmentation header size */
> +#define RTE_IPV6_FRAG_HDR_SIZE 8
> +
> +/**
> + * Parse next IPv6 header extension
> + *
> + * This function checks if proto number is an IPv6 extensions and parses its
> + * data if so, providing information on next header and extension length.
> + *
> + * @param p
> + *   Pointer to an extension raw data.
> + * @param proto
> + *   Protocol number extracted from the "next header" field from
> + *   the IPv6 header or the previous extension.
> + * @param ext_len
> + *   Extension data length.
> + * @return
> + *   next protocol number if proto is an IPv6 extension, -EINVAL otherwise
> + */
> +static inline int __rte_experimental
> +rte_ipv6_get_next_ext(uint8_t *p, int proto, size_t *ext_len)
> +{
> +	int next_proto;
> +
> +	switch (proto) {
> +	case IPPROTO_AH:
> +		next_proto = *p++;
> +		*ext_len = (*p + 2) * sizeof(uint32_t);
> +		break;
> +
> +	case IPPROTO_HOPOPTS:
> +	case IPPROTO_ROUTING:
> +	case IPPROTO_DSTOPTS:
> +		next_proto = *p++;
> +		*ext_len = (*p + 1) * sizeof(uint64_t);
> +		break;
> +
> +	case IPPROTO_FRAGMENT:
> +		next_proto = *p;
> +		*ext_len = RTE_IPV6_FRAG_HDR_SIZE;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return next_proto;
> +}
> +
>  #ifdef __cplusplus
>  }
>  #endif

I just want to highlight the potential danger of this kind of function: then
length is read from the packet (controlled by the peer), and returned without
check.

My initial fear was that an attacker forge an IPv6 packet, resulting in *ext_len
being 0.  For instance, calling the function with (proto = IPPROTO_ROUTING,
buffer = [ 0, 255, ... ]). Fortunatly, this does not happen, due to the implicit
promotion of *p to a larger integer in the (*p + 1) expression.

That said, what about adding some comments in the description, saying that:
- the pointer p must point to a buffer whose length is at least 2
- the returned length must be checked by the caller to ensure it does not
  overflow the packet buffer

You can add my ack in the next version.

Thanks,
Olivier