On 2015-04-28 17:00, Krzysztof Opasiak wrote:
> Hi Stefan,
>
> On 04/28/2015 01:51 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
>>
>> Additionally, get_ether_addr potentially 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.
>>
>> Signed-off-by: Stefan Agner <stefan@xxxxxxxx>
>> ---
>> drivers/usb/gadget/function/u_ether.c | 41 +++++++++++++++++++++--------------
>> 1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
>> index f1fd777..9994645 100644
>> --- a/drivers/usb/gadget/function/u_ether.c
>> +++ b/drivers/usb/gadget/function/u_ether.c
>> @@ -705,23 +705,27 @@ static int eth_stop(struct net_device *net)
>>
>> static int get_ether_addr(const char *str, u8 *dev_addr)
>> {
>> - if (str) {
>> - unsigned i;
>> + int num, i = 0;
>> + char c = *str;
>>
>> - for (i = 0; i < 6; i++) {
>> - unsigned char num;
>> -
>> - if ((*str == '.') || (*str == ':'))
>> - str++;
>> - num = hex_to_bin(*str++) << 4;
>> - num |= hex_to_bin(*str++);
>> - dev_addr [i] = num;
>> + dev_addr[i] = 0;
>> + while (c && i < ETH_ALEN) {
>> + if (c == '.' || c == ':') {
>> + dev_addr[++i] = 0;
>> + } else {
>> + num = hex_to_bin(c);
>> + if (num < 0)
>> + break;
>> + dev_addr[i] <<= 4;
>> + dev_addr[i] |= num;
>> }
>> - if (is_valid_ether_addr(dev_addr))
>> - return 0;
>> + c = *++str;
>> }
>> - eth_random_addr(dev_addr);
>> - return 1;
>> +
>> + if ((i + 1) != ETH_ALEN || !is_valid_ether_addr(dev_addr))
>> + return -EINVAL;
>> +
>> + return 0;
>> }
>>
>
> First of all thank you for your remark about reading past the buffer
> in my version of this patch. I tried to make my change as little as
> possible and didn't catch that this function is broken from beginning.
>
> This one is definitely better in this case but I'm afraid that it
> won't be able to correctly interpret MAC address written without
> separators while original version works fine for it.
That is true, we need separators to be able to parse the address now. Do
we need to support the format without separators? The sysfs
documentation is not very clear about the supported format (e.g.
Documentation/ABI/testing/configfs-usb-gadget-ecm).
Having variable length of the individual bytes and no separators for the
individual bytes somehow contradicts. We can of course just assume that
we have 2 characters when using no separators, its just somewhat
irregular...
> I'm also not sure if it is a good idea to remove random address
> generation from this function. Please see my remark below.
Yes I'm aware of that.
>
>> static int get_ether_addr_str(u8 dev_addr[ETH_ALEN], char *str, int len)
>> @@ -786,12 +790,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);
>>
>
> This function is also called in a few other places. Is there some
> reason why you didn't change it? For example in gether_set_dev_addr()
> which is used in many gadget modules.
The other functions made no use of the random address, hence I moved
that function call from the common function get_ether_addr to the
callers. For instance gether_set_dev_addr, if 1 has been returned by
get_ether_addr, the result had been ignored anyway.
So this is an additional optimization, which avoids generating random
addresses when not used anyway.
--
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
