[PATCH v3] usb: gadget: ether: Fix MAC address parsing

[PATCH v3] usb: gadget: ether: Fix MAC address parsing

[Stefan Agner]

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@agner.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;
+
+			num = hex_to_bin(c);
+			if (num < 0)
+				return num;
+
+			dev_addr[i] <<= 4;
+			dev_addr[i] |= num;
+			j++;
+			str++;
 		}
-		if (is_valid_ether_addr(dev_addr))
-			return 0;
 	}
-	eth_random_addr(dev_addr);
-	return 1;
+
+	if (!is_valid_ether_addr(dev_addr))
+		return -EINVAL;
+
+	return 0;
 }
 
 static int get_ether_addr_str(u8 dev_addr[ETH_ALEN], char *str, int len)
@@ -786,12 +823,17 @@ struct eth_dev *gether_setup_name(struct usb_gadget *g,
 	dev->qmult = qmult;
 	snprintf(net->name, sizeof(net->name), "%s%%d", netname);
 
-	if (get_ether_addr(dev_addr, net->dev_addr))
+	if (get_ether_addr(dev_addr, net->dev_addr)) {
+		eth_random_addr(net->dev_addr);
 		dev_warn(&g->dev,
 			"using random %s ethernet address\n", "self");
-	if (get_ether_addr(host_addr, dev->host_mac))
+	}
+
+	if (get_ether_addr(host_addr, dev->host_mac)) {
+		eth_random_addr(dev->host_mac);
 		dev_warn(&g->dev,
 			"using random %s ethernet address\n", "host");
+	}
 
 	if (ethaddr)
 		memcpy(ethaddr, dev->host_mac, ETH_ALEN);
-- 
2.4.0

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

[Krzysztof Opasiak]

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@agner.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.

Best regards,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

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

[Stefan Agner]

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@agner.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