linux-kernel - Re: [PATCH v3] usb: gadget: ether: Fix MAC address parsing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 18 May 2015 13:40:17 +0200
From:	Stefan Agner <stefan@...er.ch>
To:	Krzysztof Opasiak <k.opasiak@...sung.com>
Cc:	balbi@...com, gregkh@...uxfoundation.org, andrzej.p@...sung.com,
	m.szyprowski@...sung.com, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] usb: gadget: ether: Fix MAC address parsing

On 2015-05-18 11:24, Krzysztof Opasiak wrote:
> Hello,
> 
> On 05/14/2015 06:50 PM, Stefan Agner wrote:
>> MAC addresses can be written without leading zeros. A popular
>> example is libc's ether_ntoa_r function which creates such
>> MAC addresses.
>>
>> Example:
>> 00:14:3d:0f:ff:fe can be written as 0:14:3d:f:ff:fe
>>
>> The function get_ether_addr potentially also parsed past the
>> end of the user provided string. Use the opportunity and fix
>> the function to never parse beyond the end of the string while
>> allowing MAC addresses with and without leading zeros. Also
>> corner cases such as 00:14:3d:0f:ff:0 + new-line character
>> are parsed correctly.
>>
>> Signed-off-by: Stefan Agner <stefan@...er.ch>
>> ---
>> Changes since v2:
>> - Fix parameter description
>> - Return with error if invalid charaters are part of the string
>>
>>   drivers/usb/gadget/function/u_ether.c | 70 ++++++++++++++++++++++++++++-------
>>   1 file changed, 56 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
>> index f1fd777..c71ab21 100644
>> --- a/drivers/usb/gadget/function/u_ether.c
>> +++ b/drivers/usb/gadget/function/u_ether.c
>> @@ -703,25 +703,62 @@ static int eth_stop(struct net_device *net)
>>
>>   /*-------------------------------------------------------------------------*/
>>
>> +/**
>> + * get_ether_addr - parse ethernet address from string
>> + * @str: string to parse
>> + * @dev_addr: a buffer in which the parsed ethernet address will be
>> + *      written
>> + *
>> + * Several common formats are supported:
>> + * 1) 00:14:3d:0f:ff:fe (no skipped 0, semicolons)
>> + * 2) 00.14.3d.0f.ff.fe (no skipped 0, dots)
>> + * 3) 00143d0ffffe (no skipped 0, no separator)
>> + * 4) 0:14:3d:f:ff:fe (skipped leading 0, semicolons)
>> + * 5) 0.14.3d.f.ff.fe (skipped leading 0, dots)
>> + *
>> + * The function will not cross the end of the provided string even
>> + * when the string has the wrong format.
>> + *
>> + * Returns 0 on success, or -EINVAL on error
>> + */
>>   static int get_ether_addr(const char *str, u8 *dev_addr)
>>   {
>> -	if (str) {
>> -		unsigned	i;
>> +	int num, i;
>>
>> -		for (i = 0; i < 6; i++) {
>> -			unsigned char num;
>> +	for (i = 0; i < ETH_ALEN; i++) {
>> +		int j = 0;
>>
>> -			if ((*str == '.') || (*str == ':'))
>> +		dev_addr[i] = 0;
>> +		while (*str) {
>> +			char c = *str;
>> +
>> +			if (c == '.' || c == ':') {
>>   				str++;
>> -			num = hex_to_bin(*str++) << 4;
>> -			num |= hex_to_bin(*str++);
>> -			dev_addr [i] = num;
>> +				break;
>> +			}
>> +
>> +			if (j >= 2)
>> +				break;
>> +
>> +			/* Ignore newline character, e.g. when using echo */
>> +			if (c == '\n')
>> +				continue;
> 
> Unfortunately this line causes livelock if user will place newline
> character in the middle of MAC address. For example:
> 
> echo -e "0.14.3d.f\n\n\n.f\nf.fe" > host_addr
> 
> When you find \n character you are going once again through loop but
> you are not moving to next character so c is still \n and will never
> change to anything else as it is incremented in very last line of this
> loop.
> 
> In my opinion we should accept \n only at the end of input.
> 
>> +
>> +			num = hex_to_bin(c);
>> +			if (num < 0)
>> +				return num;
>> +
>> +			dev_addr[i] <<= 4;
>> +			dev_addr[i] |= num;
>> +			j++;
>> +			str++;
>>   		}
> 
> I'm also not sure if it is a good idea to place 0 in mac address if
> string is too short. For example:
> 
> echo -n 0.14.3d.f.ff
> 
> which is too short MAC address (last byte is missing) will be parsed
> as 00:14:3d:0f:ff:00 and used as correct MAC address. If this is the
> way it should be, maybe place at least a comment about it as it was
> not obvious (at least for me) that too short mac address should be
> also accepted.

Hm, that worked before in a totally unclean way, since the old
implementation just parsed past the end of the string. Hence random
bytes could have appeared. I think we really should fail if the MAC
address provided is too short. Will update this.

--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/