linux-kernel - Re: [PATCH v4 4/6] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

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

Date:   Mon, 25 Mar 2019 11:46:31 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, mingo@...nel.org,
        keescook@...omium.org, kirill@...temov.name,
        yamada.masahiro@...ionext.com, bp@...en8.de, hpa@...or.com,
        dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org,
        x86@...nel.org, thgarnie@...gle.com
Subject: Re: [PATCH v4 4/6] x86/mm/KASLR: Fix the wrong calculation of memory
 region initial size

On 03/24/19 at 09:58pm, Thomas Gleixner wrote:
> On Thu, 14 Mar 2019, Baoquan He wrote:
> > In memory region KASLR, __PHYSICAL_MASK_SHIFT is taken to calculate
> > the initial size of the direct mapping region. This is correct in
> > the old code where __PHYSICAL_MASK_SHIFT was equal to MAX_PHYSMEM_BITS,
> > 46 bits, and only 4-level mode was supported.
> > 
> > Later, in commit:
> > b83ce5ee91471d ("x86/mm/64: Make __PHYSICAL_MASK_SHIFT always 52"),
> > __PHYSICAL_MASK_SHIFT was changed to be always 52 bits, no matter it's
> > 5-level or 4-level. This is wrong for 4-level paging. Then when we
> > adapt physical memory region size based on available memory, it
> > will overflow if the amount of system RAM and the padding is bigger
> > than 64 TB.
> 
> I have no idea what that sentence means and what will overflow. Neither I
> have the time to stare at the code to figure it out. Changelogs need to be
> self explanatory. Providing a simple example with numbers or an
> illustration would be helpful.
> 
> > In fact, here MAX_PHYSMEM_BITS should be used instead. Fix it by
> > replacing __PHYSICAL_MASK_SHIFT with MAX_PHYSMEM_BITS.
> > 
> > Signed-off-by: Baoquan He <bhe@...hat.com>
> > Acked-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> > Reviewed-by: Thomas Garnier <thgarnie@...gle.com>
> > Acked-by: Kees Cook <keescook@...omium.org>
> 
> So this is an actual bug fix, which is in the middle of this series. Bug
> fixes go first and need to be independent of the rest of the series.
> 
> They also need a Fixes: tag.
> 
> > diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> > index d7ea6b252594..ebf6d1d92385 100644
> > --- a/arch/x86/mm/kaslr.c
> > +++ b/arch/x86/mm/kaslr.c
> > @@ -131,7 +131,7 @@ void __init kernel_randomize_memory(void)
> >  	if (!kaslr_memory_enabled())
> >  		return;
> >  
> > -	kaslr_regions[0].size_tb = 1 << (__PHYSICAL_MASK_SHIFT - TB_SHIFT);
> > +	kaslr_regions[0].size_tb = 1 << (MAX_PHYSMEM_BITS - TB_SHIFT);
> >  	kaslr_regions[1].size_tb = VMALLOC_SIZE_TB;
> 
> That said, I surely can understand why this change needs to be done, but
> the changelog needs to explain the issue so someone with less experience or
> someone looking at this in a year from now doesn't have to bang his head
> against the wall.

OK, let me add example into log and make log more understandable.
Thanks.

I will take out patch 4, 5, 6 of this series and send them out
separately. Then send patch 1, 2 ,3 as a clean up patch series.