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@xxxxxxxx>
>> ---
>> 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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
