From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>,
Aaron Conole <aconole@redhat.com>
Cc: dev@dpdk.org, Olivier Matz <olivier.matz@6wind.com>,
Andrew Rybchenko <arybchenko@solarflare.com>,
Michael Santana <msantana@redhat.com>
Subject: Re: [dpdk-dev] [PATCH] rte_ether: force format string for unformat_addr
Date: Fri, 19 Jul 2019 18:59:16 +0100 [thread overview]
Message-ID: <8996d225-7b52-1adb-3f4b-617c2fcad986@intel.com> (raw)
In-Reply-To: <20190717114200.0f2e79d4@xps13>
On 7/17/2019 7:42 PM, Stephen Hemminger wrote:
> 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
>
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'
next prev parent reply other threads:[~2019-07-19 17:59 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-10 18:33 [dpdk-dev] [PATCH] rte_ether: force format string for unformat_addr 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
2019-07-19 17:59 ` Ferruh Yigit [this message]
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=8996d225-7b52-1adb-3f4b-617c2fcad986@intel.com \
--to=ferruh.yigit@intel.com \
--cc=aconole@redhat.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=msantana@redhat.com \
--cc=olivier.matz@6wind.com \
--cc=stephen@networkplumber.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.