From: Stephen Hemminger <stephen@networkplumber.org>
To: Aaron Conole <aconole@redhat.com>
Cc: dev@dpdk.org, Olivier Matz <olivier.matz@6wind.com>,
Andrew Rybchenko <arybchenko@solarflare.com>,
Ferruh Yigit <ferruh.yigit@intel.com>,
Michael Santana <msantana@redhat.com>
Subject: Re: [dpdk-dev] [PATCH] rte_ether: force format string for unformat_addr
Date: Wed, 17 Jul 2019 11:42:00 -0700 [thread overview]
Message-ID: <20190717114200.0f2e79d4@xps13> (raw)
In-Reply-To: <f7t5zo9oclc.fsf@dhcp-25.97.bos.redhat.com>
On Wed, 10 Jul 2019 16:31:59 -0400
Aaron Conole <aconole@redhat.com> wrote:
> Stephen Hemminger <stephen@networkplumber.org> writes:
>
> > On Wed, 10 Jul 2019 15:13:02 -0400
> > Aaron Conole <aconole@redhat.com> wrote:
> >
> >> Stephen Hemminger <stephen@networkplumber.org> writes:
> >>
> >> > On Wed, 10 Jul 2019 14:33:42 -0400
> >> > Aaron Conole <aconole@redhat.com> 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 <msantana@redhat.com>
> >> >> Fixes: 596d31092d32 ("net: add function to convert string to ethernet address")
> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> >> ---
> >> >> 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
next prev parent reply other threads:[~2019-07-17 18:42 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-10 18:33 Aaron Conole
2019-07-10 18:42 ` Stephen Hemminger
2019-07-10 19:13 ` Aaron Conole
2019-07-10 19:27 ` Stephen Hemminger
2019-07-10 20:31 ` Aaron Conole
2019-07-10 23:13 ` Stephen Hemminger
2019-07-17 18:42 ` Stephen Hemminger [this message]
2019-07-19 17:59 ` Ferruh Yigit
2019-07-21 17:32 ` Aaron Conole
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190717114200.0f2e79d4@xps13 \
--to=stephen@networkplumber.org \
--cc=aconole@redhat.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=msantana@redhat.com \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).