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 2A3A2A046B for ; Sun, 21 Jul 2019 19:32:17 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CE17B2BA8; Sun, 21 Jul 2019 19:32:15 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id C2FE8152A for ; Sun, 21 Jul 2019 19:32:13 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 08A0630842B5; Sun, 21 Jul 2019 17:32:13 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-121-243.rdu2.redhat.com [10.10.121.243]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C5C65600C6; Sun, 21 Jul 2019 17:32:11 +0000 (UTC) From: Aaron Conole To: Ferruh Yigit Cc: Stephen Hemminger , dev@dpdk.org, Olivier Matz , Andrew Rybchenko , Michael Santana References: <20190710183342.6459-1-aconole@redhat.com> <20190710114230.7c171c7a@hermes.lan> <20190710122756.62ec19a9@hermes.lan> <20190717114200.0f2e79d4@xps13> <8996d225-7b52-1adb-3f4b-617c2fcad986@intel.com> Date: Sun, 21 Jul 2019 13:32:10 -0400 In-Reply-To: <8996d225-7b52-1adb-3f4b-617c2fcad986@intel.com> (Ferruh Yigit's message of "Fri, 19 Jul 2019 18:59:16 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Sun, 21 Jul 2019 17:32:13 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] rte_ether: force format string for 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" Ferruh Yigit writes: > On 7/17/2019 7:42 PM, Stephen Hemminger wrote: >> On Wed, 10 Jul 2019 16:31:59 -0400 >> Aaron Conole wrote: >> >>> Stephen Hemminger writes: >>> >>>> On Wed, 10 Jul 2019 15:13:02 -0400 >>>> Aaron Conole wrote: >>>> >>>>> Stephen Hemminger writes: >>>>> >>>>>> On Wed, 10 Jul 2019 14:33:42 -0400 >>>>>> Aaron Conole wrote: >>>>>> >>>>>>> rte_ether_unformation_addr is very lax in what it accepts now, including >>>>>>> ethernet addresses formatted ambiguously as "x:xx:x:xx:x:xx". However, >>>>>>> previously this behavior was enforced via the my_ether_aton which would >>>>>>> fail ambiguously formatted values. >>>>>>> >>>>>>> Reported-by: Michael Santana >>>>>>> Fixes: 596d31092d32 ("net: add function to convert string to ethernet address") >>>>>>> Signed-off-by: Aaron Conole >>>>>>> --- >>>>>>> lib/librte_net/rte_ether.c | 6 ++++-- >>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c >>>>>>> index 8d040173c..4f252b813 100644 >>>>>>> --- a/lib/librte_net/rte_ether.c >>>>>>> +++ b/lib/librte_net/rte_ether.c >>>>>>> @@ -45,7 +45,8 @@ rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea) >>>>>>> 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) { >>>>>>> + o3 > UINT8_MAX || o4 > UINT8_MAX || o5 > UINT8_MAX || >>>>>>> + strlen(s) != RTE_ETHER_ADDR_FMT_SIZE - 1) { >>>>>>> rte_errno = ERANGE; >>>>>>> return -1; >>>>>>> } >>>>>>> @@ -58,7 +59,8 @@ rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea) >>>>>>> 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) { >>>>>>> + if (o0 > UINT16_MAX || o1 > UINT16_MAX || o2 > UINT16_MAX || >>>>>>> + strlen(s) != RTE_ETHER_ADDR_FMT_SIZE - 4) { >>>>>>> rte_errno = ERANGE; >>>>>>> return -1; >>>>>>> } >>>>>> >>>>>> NAK >>>>>> Skipping leading zero should be ok. There is no need for this patch. >>>>> >>>>> Is it intended to skip the leading 0? Why not the trailing 0? I'm not >>>>> familiar with the format that is used here (example - X:XX:X:XX:X) >>>>> >>>>> It isn't described in any RFC I could find (but I only did a small >>>>> search). Even in IEEE, the format is always a full octet. >>>>> >>>>>> The current behavior is superset of what standard ether_aton accepts. >>>>> >>>>> Okay, but it introduces a test failure for the cmdline tests and then >>>>> that test will need a few lines removed for 'unsuccessful' formats. >>>>> >>>>> ether_aton is much more rigid in the formats it accepts, so the test >>>>> case is enforcing that. I guess either the current behavior of this >>>>> function changes (and since it is a new behavior of the cmdline parser, >>>>> I would think it should be changed) or the test case should be changed >>>>> to adopt it. >>>> >>>> BSD ether_aton is: >>>> /* >>>> * Convert an ASCII representation of an ethernet address to binary form. >>>> */ >>>> struct ether_addr * >>>> ether_aton_r(const char *a, struct ether_addr *e) >>>> { >>>> int i; >>>> unsigned int o0, o1, o2, o3, o4, o5; >>>> >>>> i = sscanf(a, "%x:%x:%x:%x:%x:%x", &o0, &o1, &o2, &o3, &o4, &o5); >>>> if (i != 6) >>>> return (NULL); >>>> e->octet[0]=o0; >>>> e->octet[1]=o1; >>>> e->octet[2]=o2; >>>> e->octet[3]=o3; >>>> e->octet[4]=o4; >>>> e->octet[5]=o5; >>>> return (e); >>>> } >>> >>> Your implementation fixes the above by bounds checking each octet >>> to enforce that in the 6-octet form, each octet is bound to the region >>> 00-ff. >>> >>> The BSD example only accepts a 6-octet form. Your version is intended >>> to accept both colon forms so x:x:x will successfully parse as well >>> (interpreted on the XXXX:XXXX:XXXX side) (ie: mac 02:03:04 or 2:3:4 >>> would be accepted). Further, accidentally passing an ipv6 address to >>> this routine (something a user of a cmdline interface might do) could be >>> parsed as valid (example: 2001:db8:2::1) - which would be the wrong >>> thing. I think it would be strange for length limits to be enforced in >>> cmdline parser *after* calling this, but that might be an option for >>> fixing (so patch cmdline_parse_etheraddr to do a length check after the >>> unformat_addr call). >>> >>> I guess I'm not sure what the *best* fix would be. I think the most >>> sane fix is what I've put in since it will only allow the commonly >>> accepted notation, and not allow ad-hoc accidents. Higher layers (like >>> cmdline parsers) are free to implement routines that reformat the lax >>> forms (like you might want to allow a user to pass) into more >>> restrictive forms required by a lower layer (like librte_net). I >>> concede that there could be a more friendly thing to do in some specific >>> cases - but then we must more strictly validate the *form* (ie: we >>> have a scanf where one form is a subset of another and will be okay with >>> some kinds of invalid characters being inserted - allowing for things >>> like IPV6 addresses looking like ethernet hardware addresses). >> >> >> I have a new version that is closer to original implementation >> in cmdline_parse_etheraddr. >> >> Comparison chart relative to ether_aton >> >> Input glibc BSD ORIG NEW >> 01:23:45:67:89:AB ok ok ok ok >> 4567:89AB:CDEF BAD BAD ok ok >> 00:11:22:33:44:55#garbage ok ok BAD BAD >> 00:11:22:33:44:55 garbage ok ok BAD BAD >> 0011:2233:4455#garbage BAD BAD BAD BAD >> 0123:45:67:89:AB BAD BAD BAD BAD >> 01:23:4567:89:AB BAD BAD BAD BAD >> 01:23:45:67:89AB BAD BAD BAD BAD >> 012:345:678:9AB BAD BAD BAD BAD >> 01:23:45:67:89:ABC ok ok BAD BAD >> 01:23:45:67:89:A ok ok ok BAD >> 01:23:45:67:89 BAD BAD BAD BAD >> 01:23:45:67:89:AB:CD ok ok BAD BAD >> IN:VA:LI:DC:HA:RS BAD BAD BAD BAD >> INVA:LIDC:HARS BAD BAD BAD BAD >> 01 23 45 67 89 AB BAD BAD BAD BAD >> 01-23-45-67-89-AB BAD BAD BAD BAD >> 01.23.45.67.89.AB BAD BAD BAD BAD >> 01,23,45,67,89,AB BAD BAD BAD BAD >> 01:23:45 BAD BAD ok BAD >> 01:23:45#:67:89:AB BAD BAD BAD BAD >> random invalid text BAD BAD BAD BAD >> random text BAD BAD BAD BAD >> > > Hi Aaron, > > Can you please check if you are OK after merged patch: > https://patches.dpdk.org/patch/56737/ > > If so can you please update the patch status as 'rejected' Will update the status, thanks for the reminder.