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 02466A0471 for ; Fri, 19 Jul 2019 19:59:24 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 117AA374E; Fri, 19 Jul 2019 19:59:23 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id E5D96322C for ; Fri, 19 Jul 2019 19:59:19 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Jul 2019 10:59:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,283,1559545200"; d="scan'208";a="252219487" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.10]) ([10.237.221.10]) by orsmga001.jf.intel.com with ESMTP; 19 Jul 2019 10:59:17 -0700 To: Stephen Hemminger , Aaron Conole Cc: 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> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQkI71rKFiEE 0jZTh0IuwoTjmYHH+TPrQ98TYR8FAlznMMQFCwkIBwMFFQoJCAsFFgIDAQAACgkQ+TPrQ98T YR/B9Q//a57esjq996nfZVm7AsUl7zbvhN+Ojity25ib2gcSVVsAN2j6lcQS4hf6/OVvRj3q CgebJ4o2gXR6X12UzWBJL7NE8Xpc70MvUIe0r11ykurQ9n9jUaWMjxdSqBPF93hU+Z/MZe5M 1rW5O2VJLuTJzkDw3EYUCbHOwPjeaS8Qqj3RI0LYbGthbHBIp9CsjkgsJSjTT5GQ8AQWkE7I z+hvPx6f1rllfjxFyi4DI3jLhAI+j1Nm+l+ESyoX59HrLTHAvq4RPkLpTnGBj9gOnJ+5sVEr GE0fcffsNcuMSkpqSEoJCPAHmChoLgezskhhsy0BiU3xlSIj1Dx2XMDerUXFOK3ftlbYNRte HQy4EKubfZRB8H5Rvcpksom3fRBDcJT8zw+PTH14htRApU9f8I/RamQ7Ujks7KuaB7JX5QaG gMjfPzHGYX9PfF6KIchaFmAWLytIP1t0ht8LpJkjtvUCSQZ2VxpCXwKyUzPDIF3co3tp90o7 X07uiC5ymX0K0+Owqs6zeslLY6DMxNdt8ye+h1TVkSZ5g4dCs4C/aiEF230+luL1CnejOv/K /s1iSbXQzJNM7be3FlRUz4FdwsfKiJJF7xYALSBnSvEB04R7I2P2V9Zpudkq6DRT6HZjBeJ1 pBF2J655cdoenPBIeimjnnh4K7YZBzwOLJf2c6u76fe5Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXOcvZgUJBvIWKAAKCRD5M+tD3xNhHxhBD/9toXMIaPIVFd9w1nKsRDM1GE6gZe4jie8q MJpeHB9O+936fSXA0W2X0het60wJQQ45O8TpTcxpc9nGzcE4MTaLAI3E8TjIXAO0cPqUNLyp g0DXezmTw5BU+SKZ51+jSKOtFmzJCHOJZQaMeCHD+G3CrdUHQVQBb5AeuH3KFv9ltgDcWsc8 YO70o3+tGHwcEnyXLdrI0q05wV7ncnLdkgVo+VUN4092bNMPwYly1TZWcU3Jw5gczOUEfTY7 sgo6E/sGX3B+FzgIs5t4yi1XOweCAQ/mPnb6uFeNENEFyGKyMG1HtjwBqnftbiFO3qitEIUY xWGQH23oKscv7i9lT0gg2D+ktzZhVWwHJVY/2vWSB9aCSWChcH2BT+lWrkwSpoPhy+almM84 Qz2wF72/d4ce4L27pSrS+vOXtXHLGOOGcAn8yr9TV0kM4aR+NbGBRXGKhG6w4lY54uNd9IBa ARIPUhij5JSygxZCBaJKo+X64AHGkk5bXq+f0anwAMNuJXbYC/lz4DEdKmPgQGShOWNs1Y1a N3cI87Hun/RBVwQ0a3Tr1g6OWJ6xK8cYbMcoR8NZ7L9ALMeJeuUDQR39+fEeHg/6sQN0P0mv 0sL+//BAJphCzDk8ztbrFw+JaPtgzZpRSM6JhxnY+YMAsatJRXA0WSpYP5zzl7yu/GZJIgsv VQ== Message-ID: <8996d225-7b52-1adb-3f4b-617c2fcad986@intel.com> Date: Fri, 19 Jul 2019 18:59:16 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190717114200.0f2e79d4@xps13> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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" 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'