linux-kernel - Re: [PATCH v2 0/2] x86/mm/KASLR: Change the granularity of randomization to PUD size in 5-level

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: <20190228130144.GT14858@MiWiFi-R3L-srv>
Date:   Thu, 28 Feb 2019 21:01:44 +0800
From:   Baoquan He <bhe@...hat.com>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
Cc:     linux-kernel@...r.kernel.org, kirill.shutemov@...ux.intel.com,
        dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        x86@...nel.org, keescook@...omium.org, thgarnie@...gle.com
Subject: Re: [PATCH v2 0/2] x86/mm/KASLR: Change the granularity of
 randomization to PUD size in 5-level

On 02/28/19 at 01:30pm, Kirill A. Shutemov wrote:
> > diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> > index 754b5da91d43..131e08a10a68 100644
> > --- a/arch/x86/mm/kaslr.c
> > +++ b/arch/x86/mm/kaslr.c
> > @@ -226,74 +226,68 @@ void __init kernel_randomize_memory(void)
> >  
> >  static void __meminit init_trampoline_pud(void)
> >  {
> > -	unsigned long paddr, paddr_next;
> > +	unsigned long paddr, vaddr;
> >  	pgd_t *pgd;
> > -	pud_t *pud_page, *pud_page_tramp;
> > -	int i;
> > +
> 
> Unneeded empty line.

Right, I will remove it, and move it to the top since it's the longest
line.

> 
> > +	pud_t *pud_page, *pud_page_tramp, *pud, *pud_tramp;
> >  
> >  	pud_page_tramp = alloc_low_page();
> >  
> >  	paddr = 0;
> > +	vaddr = (unsigned long)__va(paddr);
> 
> Maybe just
> 
> 	vaddr = PAGE_OFFSET;
> 
> ?

An obvious 0 to PAGE_OFFSET converting can explain the page table
borrowing from the direct mapping in some extent. While both is fine to
me if you prefer PAGE_OFFSET.

> 
> >  	pgd = pgd_offset_k((unsigned long)__va(paddr));
> 
> We do have 'vaddr' already.

Yeah, will replace it.

> 
> 	pgd = pgd_offset_k(vaddr);
> 
> > -	pud_page = (pud_t *) pgd_page_vaddr(*pgd);
> > -
> > -	for (i = pud_index(paddr); i < PTRS_PER_PUD; i++, paddr = paddr_next) {
> > -		pud_t *pud, *pud_tramp;
> > -		unsigned long vaddr = (unsigned long)__va(paddr);
> >  
> > -		pud_tramp = pud_page_tramp + pud_index(paddr);
> > -		pud = pud_page + pud_index(vaddr);
> > -		paddr_next = (paddr & PUD_MASK) + PUD_SIZE;
> > -
> > -		*pud_tramp = *pud;
> > -	}
> > +	if (pgtable_l5_enabled()) {
> > +		p4d_t *p4d_page, *p4d_page_tramp, *p4d, *p4d_tramp;
> > +		p4d_page_tramp = alloc_low_page();
> >  
> > -	set_pgd(&trampoline_pgd_entry,
> > -		__pgd(_KERNPG_TABLE | __pa(pud_page_tramp)));
> > -}
> > +		p4d_page = (p4d_t *) pgd_page_vaddr(*pgd);
> > +		p4d = p4d_page + p4d_index(vaddr);
> >  
> > -static void __meminit init_trampoline_p4d(void)
> > -{
> > -	unsigned long paddr, paddr_next;
> > -	pgd_t *pgd;
> > -	p4d_t *p4d_page, *p4d_page_tramp;
> > -	int i;
> > +		pud_page = (pud_t *) p4d_page_vaddr(*p4d);
> > +		pud = pud_page + pud_index(vaddr);
> >  
> > -	p4d_page_tramp = alloc_low_page();
> > +		p4d_tramp = p4d_page_tramp + p4d_index(paddr);
> > +		pud_tramp = pud_page_tramp + pud_index(paddr);
> 
> p?d_index() has to be called on the virtual address. It shouldn't break
> anything, but it's wrong from conceptual point of view.
> I don't think need paddr at all in the function.

Hmm, here the tramp table is for identity mapping real mode. Its paddr
equals to its vaddr. Using paddr here can tell it's the identity mapping
which is handling.

> 
> BTW, any reason we cannot use p?d_offset() instead of playing with
> p?d_index() directly?

Sure, p?d_offset() looks better. Just took it from the old code. I will
use it instead.

Thanks for careful reviewing.