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

Powered by blists - more mailing lists