Re: [PATCH net-next] dev: Correctly get length of alias string in dev_set_alias()

[Serhey Popovich]

David Miller wrote:
> From: Serhey Popovych 
> Date: Mon, 18 Dec 2017 23:38:35 +0200
> 
>> We supply number of bytes available in @alias via @len
>> parameter to dev_set_alias() which is not the same
>> as zero terminated string length that can be shorter.
>>
>> Both dev_set_alias() users (rtnetlink and sysfs) can
>> submit number of bytes up to IFALIASZ with actual string
>> length slightly shorter by putting '\0' not at @len - 1.
>>
>> Use strnlen() to get length of zero terminated string
>> and not access beyond @len. Correct comment about @len
>> and explain how to unset alias (i.e. use zero for @len).
>>
>> Signed-off-by: Serhey Popovych 
> 
> I don't really see this as useful, really.
> 
> In the sysfs case, we are not presented with a NULL terminated string.
> Instead, the net sysfs code gives us a length that goes up until the
> trailing newline character.  The sysfs case is never larger than the
> actual string size + 1.
> 
> The netlink attribute is usually sized appropriately for whatever the
> string length actually is.

Sorry but I do not mean larger. I mean shorter. When nla_len() >
strlen() we allocate extra space up to IFALIASZ - 1.

This is definitely fix nothing: we never get above the bounds, but
in case if NULL terminator is in the middle of string with nla_len()
we might allocate unused extra space.

Sorry again if I'm not correct with above assumption.

> 
> This therefore just seems to add an new strnlen() unnecessarily to
> this code path, which rarely does anything helpful.
> 
> Thanks.
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Serhey Popovich]

Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 23:37:09 +0200
> Serhey Popovich <serhe.popov...@gmail.com> wrote:
> 
>> Stephen Hemminger wrote:
>>> On Mon, 18 Dec 2017 23:02:07 +0200
>>> Serhey Popovich <serhe.popov...@gmail.com> wrote:
>>>   
>>>> Stephen Hemminger wrote:  
>>>>> On Mon, 18 Dec 2017 20:54:06 +0200
>>>>> Serhey Popovych <serhe.popov...@gmail.com> wrote:
>>>>> 
>>>>>> diff --git a/ip/iplink.c b/ip/iplink.c
>>>>>> index 1e685cc..4f9c169 100644
>>>>>> --- a/ip/iplink.c
>>>>>> +++ b/ip/iplink.c
>>>>>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct 
>>>>>> iplink_req *req,
>>>>>>  *name = *argv;
>>>>>>  } else if (strcmp(*argv, "index") == 0) {
>>>>>>  NEXT_ARG();
>>>>>> +if (*index)
>>>>>> +duparg("index", *argv);
>>>>>>  *index = atoi(*argv);
>>>>>> -if (*index < 0)
>>>>>> +if (*index <= 0)
>>>>>
>>>>> Why not use strtoul instead of atoi?
>>>> Do not see reason for strtoul() instead atoi():
>>>>
>>>>   1) main arg: indexes in kernel represented as "int", which is
>>>>  signed. <= 0 values are reserved for various special purposes
>>>>  (see net/core/fib_rules.c on how device matching implemented).
>>>>
>>>>  Configuring network device manually with index <= 0 is not correct
>>>>  (however possible). Kernel itself never chooses ifindex <= 0.
>>>>
>>>>  Having unsigned int > 0x7fff actually means index <= 0.
>>>>
>>>>   2) this is not single place in iproute2 where it is used: not
>>>>  going to remove last user.
>>>>
>>>>   3) make changes clear and transparent for review.  
>>>
>>> I would rather all of iproute2 correctly handles unsigned values.
>>> Too much code is old K style C "the world is an int" and "who needs
>>> to check for negative".  
>>
>> You are right :(. I'm just trying to improve things a bit.
>>
>>>
>>> There already is get_unsigned() in iproute2 util functions.  
>> This is good one based on strtoul(). But do we want to submit say
>> index = (unsigned int)2147483648(0x7fff) to the kernel that is
>> illegal from it's perspective?
>>
>> Or do you mean I can prepare treewide change to replace atoi() with
>> get_unsigned()/get_integer() where appropriate?
>>
>> We already check if (*index < 0) since commit 3c682146aeff
>> (iplink: forbid negative ifindex and modifying ifindex), and I just
>> put index == 0 in the same range of invalid indexes.
>>
> 
> The legacy BSD ABI for interfaces uses int, so that sets the upper
> bound for kernel.
> 
> The netlink ABI limit is u32 for ifindex so technically 1..UINT32_MAX are
> possible values but kernel is bound by BSD mistake.
Thank you for in depth explanation!

> 
> I will take the original patch.
> 
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Serhey Popovich]

Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 23:02:07 +0200
> Serhey Popovich <serhe.popov...@gmail.com> wrote:
> 
>> Stephen Hemminger wrote:
>>> On Mon, 18 Dec 2017 20:54:06 +0200
>>> Serhey Popovych <serhe.popov...@gmail.com> wrote:
>>>   
>>>> diff --git a/ip/iplink.c b/ip/iplink.c
>>>> index 1e685cc..4f9c169 100644
>>>> --- a/ip/iplink.c
>>>> +++ b/ip/iplink.c
>>>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct 
>>>> iplink_req *req,
>>>>*name = *argv;
>>>>} else if (strcmp(*argv, "index") == 0) {
>>>>NEXT_ARG();
>>>> +  if (*index)
>>>> +  duparg("index", *argv);
>>>>*index = atoi(*argv);
>>>> -  if (*index < 0)
>>>> +  if (*index <= 0)  
>>>
>>> Why not use strtoul instead of atoi?  
>> Do not see reason for strtoul() instead atoi():
>>
>>   1) main arg: indexes in kernel represented as "int", which is
>>  signed. <= 0 values are reserved for various special purposes
>>  (see net/core/fib_rules.c on how device matching implemented).
>>
>>  Configuring network device manually with index <= 0 is not correct
>>  (however possible). Kernel itself never chooses ifindex <= 0.
>>
>>  Having unsigned int > 0x7fff actually means index <= 0.
>>
>>   2) this is not single place in iproute2 where it is used: not
>>  going to remove last user.
>>
>>   3) make changes clear and transparent for review.
> 
> I would rather all of iproute2 correctly handles unsigned values.
> Too much code is old K style C "the world is an int" and "who needs
> to check for negative".

You are right :(. I'm just trying to improve things a bit.

> 
> There already is get_unsigned() in iproute2 util functions.
This is good one based on strtoul(). But do we want to submit say
index = (unsigned int)2147483648(0x7fff) to the kernel that is
illegal from it's perspective?

Or do you mean I can prepare treewide change to replace atoi() with
get_unsigned()/get_integer() where appropriate?

We already check if (*index < 0) since commit 3c682146aeff
(iplink: forbid negative ifindex and modifying ifindex), and I just
put index == 0 in the same range of invalid indexes.

> Why not use that?
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling

[Serhey Popovich]

Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 20:54:06 +0200
> Serhey Popovych  wrote:
> 
>> diff --git a/ip/iplink.c b/ip/iplink.c
>> index 1e685cc..4f9c169 100644
>> --- a/ip/iplink.c
>> +++ b/ip/iplink.c
>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struct 
>> iplink_req *req,
>>  *name = *argv;
>>  } else if (strcmp(*argv, "index") == 0) {
>>  NEXT_ARG();
>> +if (*index)
>> +duparg("index", *argv);
>>  *index = atoi(*argv);
>> -if (*index < 0)
>> +if (*index <= 0)
> 
> Why not use strtoul instead of atoi?
Do not see reason for strtoul() instead atoi():

  1) main arg: indexes in kernel represented as "int", which is
 signed. <= 0 values are reserved for various special purposes
 (see net/core/fib_rules.c on how device matching implemented).

 Configuring network device manually with index <= 0 is not correct
 (however possible). Kernel itself never chooses ifindex <= 0.

 Having unsigned int > 0x7fff actually means index <= 0.

  2) this is not single place in iproute2 where it is used: not
 going to remove last user.

  3) make changes clear and transparent for review.

Thanks.

> 




signature.asc
Description: OpenPGP digital signature