linux-kernel - Re: [RFC 1/4] net/ipv4/fib: Remove run-time check in tnode

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]

Message-ID: <b3540257a002b944a878096570bc927450662b9c.camel@linux.intel.com>
Date:   Mon, 01 Apr 2019 10:50:48 -0700
From:   Alexander Duyck <alexander.h.duyck@...ux.intel.com>
To:     Dmitry Safonov <0x7f454c46@...il.com>,
        Dmitry Safonov <dima@...sta.com>, linux-kernel@...r.kernel.org
Cc:     Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        David Ahern <dsahern@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Ido Schimmel <idosch@...lanox.com>, netdev@...r.kernel.org
Subject: Re: [RFC 1/4] net/ipv4/fib: Remove run-time check in tnode_alloc()

On Mon, 2019-04-01 at 16:55 +0100, Dmitry Safonov wrote:
> 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.

Actually now that I think about it TNODE_VMALLOC_MAX is likely much
less than 31. The logic that we have to be concerned with is:
	size = TNODE_SIZE(1ul << bits);

If size is a 32b value, and the size of a pointer is 4 bytes, then our
upper limit is roughly ilog2((4G - 28) / 4), which comes out to 29.
What we are trying to avoid is overflowing the size variable, not
actually limiting the vmalloc itself.

> > 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.

I have no problem with that. All I am suggesting is that if at all
possible we should use TNODE_VMALLOC_MAX instead of BITS_PER_LONG.

> 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.

No, the problem is we don't want to overflow size. The allocation will
succeed, but give us a much smaller allocation then we expected.

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

Sort of, however we have to keep mind that 1ul << bits is an index so
it is also increased by the size of a pointer. As such the logic might
be better expressed as sizeof(void*) << bits.