Re: [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

I'm fine with a more strict version like you proposed here. I checked
that the cmdline tests pass.

Few minor comments below.

On Wed, Jul 17, 2019 at 11:49:45AM -0700, Stephen Hemminger wrote:
> The current code acts more like BSD ether_aton and allows leading zeros
> which breaks the cmdline tests.
> 
> Change the code to be more restrictive and only allow the fully
> expanded standard formats.
> 
> Fixes: 596d31092d32 ("net: add function to convert string to ethernet address")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Can you add the bugzilla id ?
https://bugs.dpdk.org/show_bug.cgi?id=324

> ---
>  lib/librte_net/rte_ether.c | 107 ++++++++++++++++++++++++-------------
>  1 file changed, 70 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c
> index 8d040173cfc6..536449beffe4 100644
> --- a/lib/librte_net/rte_ether.c
> +++ b/lib/librte_net/rte_ether.c
> @@ -2,6 +2,8 @@
>   * Copyright(c) 2010-2014 Intel Corporation
>   */
>  
> +#include <stdbool.h>
> +
>  #include <rte_ether.h>
>  #include <rte_errno.h>
>  
> @@ -29,50 +31,81 @@ rte_ether_format_addr(char *buf, uint16_t size,
>  		 eth_addr->addr_bytes[5]);
>  }
>  
> +static int8_t get_xdigit(char ch)
> +{
> +	if (ch >= '0' && ch <= '9')
> +		return ch - '0';
> +	if (ch >= 'a' && ch <= 'f')
> +		return ch - 'a' + 10;
> +	if (ch >= 'A' && ch <= 'F')
> +		return ch - 'A' + 10;
> +	return -1;
> +}
> +
> +/* Convert 00:11:22:33:44:55 to ethernet address */
> +static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
> +{
> +	const char *s = s0;
> +	int i;
> +
> +	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> +		int8_t x;
> +
> +		x = get_xdigit(*s++);
> +		if (x < 0)
> +			return false;
> +		ea->addr_bytes[i] = x << 4;
> +		x = get_xdigit(*s++);
> +		if (x < 0)
> +			return false;
> +		ea->addr_bytes[i] |= x;

Maybe we should say in the API doc that ether address can be modified
even if parsing fails.


> +
> +		if (i < RTE_ETHER_ADDR_LEN - 1 &&
> +		    *s++ != ':')
> +			return false;
> +	}
> +	return *s == '\0';
> +}
> +
> +/* Convert 0011:2233:4455 to ethernet address */
> +static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < RTE_ETHER_ADDR_LEN; i += 2) {
> +		uint16_t w = 0;
> +
> +		for (j = 0; j < 4; j++) {
> +			int8_t x;
> +
> +			x = get_xdigit(*s++);
> +			if (x < 0)
> +				return false;
> +			w = (w << 4) | x;
> +		}
> +		ea->addr_bytes[i] = w >> 8;
> +		ea->addr_bytes[i+1] = w & 0xff;
> +
> +		if (i < RTE_ETHER_ADDR_LEN - 2 &&
> +		    *s++ != ':')
> +			return false;
> +	}
> +
> +	return *s == '\0';
> +}
> +
>  /*
>   * Like ether_aton_r but can handle either
>   * XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
> + * and is more restrictive.
>   */
>  int
>  rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea)
>  {
> -	unsigned int o0, o1, o2, o3, o4, o5;
> -	int n;
> -
> -	n = sscanf(s, "%x:%x:%x:%x:%x:%x",
> -		    &o0, &o1, &o2, &o3, &o4, &o5);
> -
> -	if (n == 6) {
> -		/* Standard format XX:XX:XX:XX:XX:XX */
> -		if (o0 > UINT8_MAX || o1 > UINT8_MAX || o2 > UINT8_MAX ||
> -		    o3 > UINT8_MAX || o4 > UINT8_MAX || o5 > UINT8_MAX) {
> -			rte_errno = ERANGE;
> -			return -1;
> -		}
> -
> -		ea->addr_bytes[0] = o0;
> -		ea->addr_bytes[1] = o1;
> -		ea->addr_bytes[2] = o2;
> -		ea->addr_bytes[3] = o3;
> -		ea->addr_bytes[4] = o4;
> -		ea->addr_bytes[5] = o5;
> -	} else if (n == 3) {
> -		/* Support the format XXXX:XXXX:XXXX */
> -		if (o0 > UINT16_MAX || o1 > UINT16_MAX || o2 > UINT16_MAX) {
> -			rte_errno = ERANGE;
> -			return -1;
> -		}
> -
> -		ea->addr_bytes[0] = o0 >> 8;
> -		ea->addr_bytes[1] = o0 & 0xff;
> -		ea->addr_bytes[2] = o1 >> 8;
> -		ea->addr_bytes[3] = o1 & 0xff;
> -		ea->addr_bytes[4] = o2 >> 8;
> -		ea->addr_bytes[5] = o2 & 0xff;
> -	} else {
> -		/* unknown format */
> -		rte_errno = EINVAL;
> +	if  (get_ether_addr6(s, ea) || get_ether_addr3(s, ea))
> +		return 0;
> +	else {
> +		rte_errno = -EINVAL;
>  		return -1;

rte_errno should be positive

>  	}
> -	return 0;
>  }
> -- 
> 2.17.1
>