Re: [dpdk-dev] [PATCH] rte_ether: force format string for unformat_addr

[Aaron Conole <aconole@redhat.com>]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> 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'

Will update the status, thanks for the reminder.