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:   Mon, 1 Apr 2019 16:55:02 +0100
From:   Dmitry Safonov <>
To:     Alexander Duyck <>,
        Dmitry Safonov <>,
Cc:     Alexey Kuznetsov <>,
        David Ahern <>,
        "David S. Miller" <>,
        Eric Dumazet <>,
        Hideaki YOSHIFUJI <>,
        Ido Schimmel <>,
Subject: Re: [RFC 1/4] net/ipv4/fib: Remove run-time check in tnode_alloc()

Hi Alexander,

On 4/1/19 4:40 PM, Alexander Duyck wrote:
>> @@ -333,8 +328,7 @@ static struct tnode *tnode_alloc(int bits)
>>  {
>>  	size_t size;
>> -	/* verify bits is within bounds */
>> -	if (bits > TNODE_VMALLOC_MAX)
>> +	if ((BITS_PER_LONG <= KEYLENGTH) && unlikely(bits >= BITS_PER_LONG))
>>  		return NULL;
>>  	/* determine size and verify it is non-zero and didn't overflow */
> I think it would be better if we left TNODE_VMALLOC_MAX instead of
> replacing it with BITS_PER_LONG. This way we know that we are limited
> by the size of the node on 32b systems, and by the KEYLENGTH on 64b
> systems. The basic idea is to maintain the logic as to why we are doing
> it this way instead of just burying things by using built in constants
> that are close enough to work.
> So for example I believe TNODE_VMALLOC_MAX is 31 on a 32b system.

This is also true after the change: bits == 31 will *not* return.

> The
> main reason for that is because we have to subtract the TNODE_SIZE from
> the upper limit for size. By replacing TNODE_VMALLOC_MAX with
> BITS_PER_LONG that becomes abstracted away and it becomes more likely
> that somebody will mishandle it later.

So, I wanted to remove run-time check here on x86_64..
I could do it by adding !CONFIG_64BIT around the check.

But, I thought about the value of the check:
I believe it's here not to limit maximum allocated size:
kzalloc()/vzalloc() will fail and we should be fine with that.

In my opinion it's rather to check that (1UL << bits) wouldn't result in UB.


Powered by blists - more mailing lists