linux-kernel - Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86

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: <20160617090213.GC4791@gmail.com>
Date:	Fri, 17 Jun 2016 11:02:13 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Kees Cook <keescook@...omium.org>
Cc:	Thomas Garnier <thgarnie@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	Borislav Petkov <bp@...e.de>, Juergen Gross <jgross@...e.com>,
	Matt Fleming <matt@...eblueprint.co.uk>,
	Toshi Kani <toshi.kani@....com>, Baoquan He <bhe@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dan Williams <dan.j.williams@...el.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Andy Lutomirski <luto@...nel.org>,
	Alexander Kuleshov <kuleshovmail@...il.com>,
	Alexander Popov <alpopov@...ecurity.com>,
	Joerg Roedel <jroedel@...e.de>, Dave Young <dyoung@...hat.com>,
	Lv Zheng <lv.zheng@...el.com>,
	Mark Salter <msalter@...hat.com>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	David Rientjes <rientjes@...gle.com>,
	Christian Borntraeger <borntraeger@...ibm.com>,
	Jan Beulich <JBeulich@...e.com>,
	Kefeng Wang <wangkefeng.wang@...wei.com>,
	Seth Jennings <sjennings@...iantweb.net>,
	Yinghai Lu <yinghai@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping
 (x86_64)


* Kees Cook <keescook@...omium.org> wrote:

> From: Thomas Garnier <thgarnie@...gle.com>
> 
> Minor change that allows early boot physical mapping of PUD level virtual
> addresses. The current implementation expects the virtual address to be
> PUD aligned. For KASLR memory randomization, we need to be able to
> randomize the offset used on the PUD table.
> 
> It has no impact on current usage.
> 
> Signed-off-by: Thomas Garnier <thgarnie@...gle.com>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  arch/x86/mm/init_64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index bce2e5d9edd4..f205f39bd808 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -454,10 +454,10 @@ phys_pud_init(pud_t *pud_page, unsigned long addr, unsigned long end,
>  {
>  	unsigned long pages = 0, next;
>  	unsigned long last_map_addr = end;
> -	int i = pud_index(addr);
> +	int i = pud_index((unsigned long)__va(addr));
>
>  
>  	for (; i < PTRS_PER_PUD; i++, addr = next) {
> -		pud_t *pud = pud_page + pud_index(addr);
> +		pud_t *pud = pud_page + pud_index((unsigned long)__va(addr));
>  		pmd_t *pmd;
>  		pgprot_t prot = PAGE_KERNEL;

So I really dislike two things about this code.

Firstly a pre-existing problem is that the parameter names to phys_pud_init() 
suck:

static unsigned long __meminit
phys_pud_init(pud_t *pud_page, unsigned long addr, unsigned long end,
                         unsigned long page_size_mask)

so 'unsigned long addr' is usually the signature of a virtual address - but that's 
no true here: it's a physical address.

Same goes for 'unsigned long end'. Plus it's unclear what the connection between 
'addr' and 'end' - it's not at all obvious 'at a glance' that they are the start 
and end addresses of a physical memory range.

All of these problems can be solved by renaming them to 'paddr_start' and 
'paddr_end'.

Btw., I believe this misnomer and confusing code resulted in the buggy 
'pud_index(addr)' not being noticed to begin with ...

Secondly, and that's a new problem introduced by this patch:

> +	int i = pud_index((unsigned long)__va(addr));
> +		pud_t *pud = pud_page + pud_index((unsigned long)__va(addr));

... beyond the repetition, using type casts is fragile. Type casts should be a red 
flag to anyone involved in low level, security relevant code! So I'm pretty 
unhappy about seeing such a problem in such a patch.

This code should be doing something like:

	unsigned long vaddr_start = __va(paddr_start);

... which gets rid of the type cast, the repetition and documents the code much 
better as well. Also see how easily the connection between the variables is 
self-documented just by picking names carefully:

	paddr_start
	paddr_end
	vaddr_start
	vaddr_end

Also, _please_ add a comment to phys_pud_init() that explains what the function 
does.

Thanks,

	Ingo