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

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

[Stephen Hemminger]

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>
---
 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;
+
+		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;
 	}
-	return 0;
 }
-- 
2.17.1

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

[Olivier Matz]

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
> 

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

[Stephen Hemminger]

On Thu, 18 Jul 2019 09:47:03 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:

> 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

Sure, will do.
I wish there was a one week delay during development and reserve Bugzilla
for stuff in released code.

> > +	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.
> 


No. Standard practice is that the state of returned data is undefined in any 
function that returns an error.

> > -	} 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

Will fix.
 

[dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats

[Stephen Hemminger]

The current ether_unformat_addr code was based off of
BSD ether_aton. That version changed what was allowed
by the cmdline ether address parser.
For example, it allows dropping leading zeros.

Change the code to be more restrictive and only allow the fully
expanded standard formats.

Bugzilla ID: 324
Fixes: 596d31092d32 ("net: add function to convert string to ethernet address")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - incorporate RFC review feedback
`
 lib/librte_net/rte_ether.c | 113 ++++++++++++++++++++++++-------------
 lib/librte_net/rte_ether.h |   3 +
 2 files changed, 78 insertions(+), 38 deletions(-)

diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c
index 8d040173cfc6..ced65ed9f3cb 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,85 @@ 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;
+
+		if (i < RTE_ETHER_ADDR_LEN - 1 &&
+		    *s++ != ':')
+			return false;
+	}
+
+	/* return true if at end of string */
+	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;
-		}
+	if (get_ether_addr6(s, ea))
+		return 0;
+	if (get_ether_addr3(s, ea))
+		return 0;
 
-		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;
-		return -1;
-	}
-	return 0;
+	rte_errno = EINVAL;
+	return -1;
 }
diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 8ac1afecc826..c2f15c56c784 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -252,6 +252,9 @@ rte_ether_format_addr(char *buf, uint16_t size,
  *
  * @param str
  *   A pointer to buffer contains the formatted MAC address.
+ *   The supported formats are:
+ *     XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
+ *   where XX is a hex digit: 0-9, a-f, or A-F.
  * @param eth_addr
  *   A pointer to a ether_addr structure.
  * @return
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats

[Olivier Matz]

On Thu, Jul 18, 2019 at 11:35:44AM -0700, Stephen Hemminger wrote:
> The current ether_unformat_addr code was based off of
> BSD ether_aton. That version changed what was allowed
> by the cmdline ether address parser.
> For example, it allows dropping leading zeros.
> 
> Change the code to be more restrictive and only allow the fully
> expanded standard formats.
> 
> Bugzilla ID: 324
> Fixes: 596d31092d32 ("net: add function to convert string to ethernet address")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats

[Ferruh Yigit]

On 7/19/2019 2:42 PM, Olivier Matz wrote:
> On Thu, Jul 18, 2019 at 11:35:44AM -0700, Stephen Hemminger wrote:
>> The current ether_unformat_addr code was based off of
>> BSD ether_aton. That version changed what was allowed
>> by the cmdline ether address parser.
>> For example, it allows dropping leading zeros.
>>
>> Change the code to be more restrictive and only allow the fully
>> expanded standard formats.
>>
>> Bugzilla ID: 324
>> Fixes: 596d31092d32 ("net: add function to convert string to ethernet address")
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
> 

Applied to dpdk-next-net/master, thanks.