netdev - Re: [PATCH iproute2 3/3] utils: ll_map: Make network device name fixed size array of char

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Dec 2017 20:17:08 +0200
From:   Serhey Popovych <serhe.popovych@...il.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH iproute2 3/3] utils: ll_map: Make network device name
 fixed size array of char

Stephen Hemminger wrote:
> On Wed, 20 Dec 2017 09:37:31 +0200
> Serhey Popovych <serhe.popovych@...il.com> wrote:
> 
>> Network device names are fixed in size and never exceed
>> IFNAMSIZ (16 bytes).
>>
>> Make name fixed size array to always malloc() same size chunk
>> of memory and use memcpy()/memcmp() with constant IFNAMSIZ
>> to benefit from possible compiler optimizations replacing
>> call to a function with two/four load/store instructions
>> on 64/32 bit systems.
>>
>> Check if IFLA_IFNAME attribute present in netlink message
>> (should always) and use strncpy() to pad name with zeros.
>>
>> Signed-off-by: Serhey Popovych <serhe.popovych@...il.com>
>> ---
>>  lib/ll_map.c |   20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/ll_map.c b/lib/ll_map.c
>> index abe7bdc..fcbf0fb 100644
>> --- a/lib/ll_map.c
>> +++ b/lib/ll_map.c
>> @@ -30,7 +30,7 @@ struct ll_cache {
>>  	unsigned	flags;
>>  	unsigned 	index;
>>  	unsigned short	type;
>> -	char		name[];
>> +	char		name[IFNAMSIZ];
>>  };
>>  
>>  #define IDXMAP_SIZE	1024
>> @@ -71,7 +71,7 @@ static struct ll_cache *ll_get_by_name(const char *name)
>>  		struct ll_cache *im
>>  			= container_of(n, struct ll_cache, name_hash);
>>  
>> -		if (strncmp(im->name, name, IFNAMSIZ) == 0)
>> +		if (!strcmp(im->name, name))
>>  			return im;
>>  	}
>>  
>> @@ -82,7 +82,7 @@ int ll_remember_index(const struct sockaddr_nl *who,
>>  		      struct nlmsghdr *n, void *arg)
>>  {
>>  	unsigned int h;
>> -	const char *ifname;
>> +	char ifname[IFNAMSIZ];
>>  	struct ifinfomsg *ifi = NLMSG_DATA(n);
>>  	struct ll_cache *im;
>>  	struct rtattr *tb[IFLA_MAX+1];
>> @@ -105,17 +105,21 @@ int ll_remember_index(const struct sockaddr_nl *who,
>>  	}
>>  
>>  	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), IFLA_PAYLOAD(n));
>> -	ifname = rta_getattr_str(tb[IFLA_IFNAME]);
>> -	if (ifname == NULL)
>> +
>> +	if (!tb[IFLA_IFNAME])
>> +		return 0;
>> +	strncpy(ifname, rta_getattr_str(tb[IFLA_IFNAME]), IFNAMSIZ);
>> +	if (!ifname[0])
>>  		return 0;
>> +	ifname[IFNAMSIZ - 1] = '\0';
>>  
>>  	if (im) {
>>  		/* change to existing entry */
>> -		rehash = strcmp(im->name, ifname);
>> +		rehash = memcmp(im->name, ifname, IFNAMSIZ);
> 
> This is not safe. There is not guarantee that bytes after end of string are zero.

Sorry Stephen, correct if my assumptions are wrong:

  1. struct ll_cache entries are only modified in ll_remember_index().
     There are no places where we may modify ll_cache entries.

  2. strncpy() always pad with zeroes to the end of IFNAMSIZ sized
     buffer.

  3. strncpy() may not return null terminated string: this addressed
     with ifname[IFNAMSIZ - 1] = '\0' in the code above.

Assuming 1 and 2 we always have im->name[] initialized with string and
zero pads up to IFNAMSIZ. We prepare ifname using strncpy() to, so it
is zero padded and we can safely use memcmp() to compare byte by byte.

> And in your code, strncpy() will overwrite characters from the beginning to null,
> it will not overwrite after that. Then comparison with entry may not work because
> of the data after that.
strncpy() will not pad with zeroes up to IFNAMSIZ? I get from strncpy(3)
it will pad to the end of buf, so IFNAMSIZ is initialized.

Please correct me if I'm wrong at some point.

Thanks.

> 
> I really doubt this is critical path on anything. Probably just having a better
> hash table implementation would solve that.
> 

Well this is critical with thousands of interfaces. I guess. Didn't go
with tests, but can do that. Of course difference might not be so big as
I expect, but anyway.



Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)