* [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr
@ 2019-07-17 18:49 Stephen Hemminger
2019-07-18 7:47 ` Olivier Matz
2019-07-18 18:35 ` [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats Stephen Hemminger
0 siblings, 2 replies; 6+ messages in thread
From: Stephen Hemminger @ 2019-07-17 18:49 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr
2019-07-17 18:49 [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr Stephen Hemminger
@ 2019-07-18 7:47 ` Olivier Matz
2019-07-18 18:29 ` Stephen Hemminger
2019-07-18 18:35 ` [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats Stephen Hemminger
1 sibling, 1 reply; 6+ messages in thread
From: Olivier Matz @ 2019-07-18 7:47 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr
2019-07-18 7:47 ` Olivier Matz
@ 2019-07-18 18:29 ` Stephen Hemminger
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2019-07-18 18:29 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats
2019-07-17 18:49 [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr Stephen Hemminger
2019-07-18 7:47 ` Olivier Matz
@ 2019-07-18 18:35 ` Stephen Hemminger
2019-07-19 13:42 ` Olivier Matz
1 sibling, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2019-07-18 18:35 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats
2019-07-18 18:35 ` [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats Stephen Hemminger
@ 2019-07-19 13:42 ` Olivier Matz
2019-07-19 17:46 ` Ferruh Yigit
0 siblings, 1 reply; 6+ messages in thread
From: Olivier Matz @ 2019-07-19 13:42 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats
2019-07-19 13:42 ` Olivier Matz
@ 2019-07-19 17:46 ` Ferruh Yigit
0 siblings, 0 replies; 6+ messages in thread
From: Ferruh Yigit @ 2019-07-19 17:46 UTC (permalink / raw)
To: Olivier Matz, Stephen Hemminger; +Cc: dev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-19 17:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-17 18:49 [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr Stephen Hemminger
2019-07-18 7:47 ` Olivier Matz
2019-07-18 18:29 ` Stephen Hemminger
2019-07-18 18:35 ` [dpdk-dev] [PATCH v2] net: be more restrictive in accepted ether string formats Stephen Hemminger
2019-07-19 13:42 ` Olivier Matz
2019-07-19 17:46 ` Ferruh Yigit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).