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 24906A00E6 for ; Thu, 11 Jul 2019 01:13:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 00F72324D; Thu, 11 Jul 2019 01:13:57 +0200 (CEST) Received: from mail-pg1-f193.google.com (mail-pg1-f193.google.com [209.85.215.193]) by dpdk.org (Postfix) with ESMTP id CE9412C23 for ; Thu, 11 Jul 2019 01:13:55 +0200 (CEST) Received: by mail-pg1-f193.google.com with SMTP id i8so1928684pgm.13 for ; Wed, 10 Jul 2019 16:13:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=rNLkf1d8rOs460DLHmH2Bg4vCl5D2Nx0BWAWHRJEqiw=; b=acqecajlW2uV6NdY96W9C2uYWKd62fay0F65oUj44GybF2BC5mh/e9F4DX9XKSC924 29EQDmsCVQytyyWzt2NCz184FOIxT4Pe9nFUUegzJBpZLh1PsKjGLDCff8Gws6UOC8kx JfHcKL3dcQAb44rW5au8vg14LOvkRPOOi3lGm8G8SKCEeGHCVFam/qhtX1WFzKLXjj+F 7jze04mqpRrWnM1I5FXZxkOBY2dEvnQvVyAKUnnEOWmyS1Zs54sTkx//oxdTt255F4eX 0gX58fYbQxCsNGzacr3HlIw6hv7GCxnqGL7abkUqoNSfqoZV2d5p779jYPUUsCbOO40B muCg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=rNLkf1d8rOs460DLHmH2Bg4vCl5D2Nx0BWAWHRJEqiw=; b=jHuCNFSKZ1VOCw4fSfA0B9iTxjJ7WOsq5jfJF7imQQI1PRfNUoWedeyPW4xq7EHmBk VZj2el3pQd5N77Kr+P//ZuLAoeYVfrOpzGJOODnuvf9qCXF52RXpQ/6J6GZR2k0Zb3GH wVUluXSBdVvZnNd3sndv7XSdrLasK7chYsFrANDAzpyTyxK6idjOb1z6GmXAhKanoKbR e9JOIweT9AQM3tp0yYazrzVN5YC6FnqEXxTbz9JPZ3laGNkYKWMMcd762qbEd9GZKddS wCqqDe9V3mTRpwm7h+XtAnNkxZK3yphIysXvElQOp/l4r9/tvjn/gPnWHg7gsOIUC7oC TmCA== X-Gm-Message-State: APjAAAV4tTukP8hD5FQJ3DkNubBNfX8N5XqIzDkhpX5l5Yq1C1hnC4Y7 D1bx8EZ2kKZf/nV1e4s87rE= X-Google-Smtp-Source: APXvYqzpvwb+ug9cm3Jvu3elyDg60CkBK2RODsz1hZNWQBfl1POSNEDl/ytP9y26YRCZd9uolIz3vw== X-Received: by 2002:a63:7a01:: with SMTP id v1mr874424pgc.310.1562800434670; Wed, 10 Jul 2019 16:13:54 -0700 (PDT) Received: from hermes.lan (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id u16sm4624915pjb.2.2019.07.10.16.13.54 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Wed, 10 Jul 2019 16:13:54 -0700 (PDT) Date: Wed, 10 Jul 2019 16:13:47 -0700 From: Stephen Hemminger To: Aaron Conole Cc: dev@dpdk.org, Olivier Matz , Andrew Rybchenko , Ferruh Yigit , Michael Santana Message-ID: <20190710161347.718e4b3c@hermes.lan> In-Reply-To: References: <20190710183342.6459-1-aconole@redhat.com> <20190710114230.7c171c7a@hermes.lan> <20190710122756.62ec19a9@hermes.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 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). Being liberal in what you accept as input is a good core principle. BSD goes too far, but what you propose is too restrictive. > 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). Fix the cmdline test.