From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0656EA0471 for ; Thu, 18 Jul 2019 09:47:08 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3F7751D9E; Thu, 18 Jul 2019 09:47:07 +0200 (CEST) Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 618951C01 for ; Thu, 18 Jul 2019 09:47:06 +0200 (CEST) Received: from lfbn-lil-1-176-160.w90-45.abo.wanadoo.fr ([90.45.26.160] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1ho1BA-0001U5-LA; Thu, 18 Jul 2019 09:50:18 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Thu, 18 Jul 2019 09:47:03 +0200 Date: Thu, 18 Jul 2019 09:47:03 +0200 From: Olivier Matz To: Stephen Hemminger Cc: dev@dpdk.org Message-ID: <20190718074703.7mjsnliicnx5eexq@platinum> References: <20190717184945.4025-1-stephen@networkplumber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190717184945.4025-1-stephen@networkplumber.org> User-Agent: NeoMutt/20180716 Subject: Re: [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "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 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 > + > #include > #include > > @@ -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 >