diff mbox series

[RFC] net: be more restrictive in ether_unformat_addr

Message ID 20190717184945.4025-1-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [RFC] net: be more restrictive in ether_unformat_addr |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger July 17, 2019, 6:49 p.m. UTC 
  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(-)

Comments

Olivier Matz July 18, 2019, 7:47 a.m. UTC | #1 
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
>
Stephen Hemminger July 18, 2019, 6:29 p.m. UTC | #2 
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.
diff mbox series

Patch

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