linux-kernel - Re: [PATCH v5 1/2] x86_64: remove not needed clear_page for init_level4

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: <20150616111632.GF17786@pd.tnic>
Date:	Tue, 16 Jun 2015 13:16:32 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Alexander Popov <alpopov@...ecurity.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrey Konovalov <adech.fo@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andrey Ryabinin <a.ryabinin@...sung.com>,
	Andy Lutomirski <luto@...nel.org>,
	Alexander Kuleshov <kuleshovmail@...il.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Kees Cook <keescook@...omium.org>, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/2] x86_64: remove not needed clear_page for
 init_level4_page

On Tue, Jun 09, 2015 at 12:42:00PM +0300, Alexander Popov wrote:
> From: Andrey Ryabinin <a.ryabinin@...sung.com>
> 
> Commit 8170e6bed465 ("x86, 64bit: Use a #PF handler to materialize
> early mappings on demand") introduced clear_page(init_level4_pgt);
> call in x86_64_start_kernel(). However, this clear_page is useless
> because init_level4_page already filled with zeroes in head_64.S

I really doubt that, see below:

> Commit message in 8170e6bed465 says that this clear_page() was
> dropped in v7, but it accidentally reappeared in later versions
> of that patchset.
> 
> Signed-off-by: Andrey Ryabinin <a.ryabinin@...sung.com>
> ---
>  arch/x86/kernel/head64.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index 5a46681..6a6eefd 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -177,7 +177,6 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
>  	 */
>  	load_ucode_bsp();
>  
> -	clear_page(init_level4_pgt);
>  	/* set init_level4_pgt kernel high mapping*/
>  	init_level4_pgt[511] = early_level4_pgt[511];

vmlinux has:

ffffffff81a00000 <init_level4_pgt>:
ffffffff81a00000:       63 10                   movslq (%rax),%edx
ffffffff81a00002:       a0 01 00 00 00 00 00    movabs 0x1,%al
ffffffff81a00009:       00 00 
        ...
ffffffff81a0087f:       00 63 10                add    %ah,0x10(%rbx)
ffffffff81a00882:       a0 01 00 00 00 00 00    movabs 0x1,%al
ffffffff81a00889:       00 00 
        ...
ffffffff81a00ff7:       00 67 30                add    %ah,0x30(%rdi)
ffffffff81a00ffa:       a0 01 00 00 00 00 63    movabs 0xa020630000000001,%al
ffffffff81a01001:       20 a0 

ffffffff81a01000 <level3_ident_pgt>:
ffffffff81a01000:       63 20                   movslq (%rax),%esp
ffffffff81a01002:       a0 01 00 00 00 00 00    movabs 0x1,%al
ffffffff81a01009:       00 00 
        ...

Booting a kvm quest and halting it before the clear_page:

---
        /*
         * Load microcode early on BSP.
         */
        load_ucode_bsp();

//      clear_page(init_level4_pgt);

        while(1)
                cpu_relax();

	/* set init_level4_pgt kernel high mapping*/
	init_level4_pgt[511] = early_level4_pgt[511];
---

and dumping the memory at ffffffff81a00000 gives:

---
00000000  63 10 a0 01 00 00 00 00  00 00 00 00 00 00 00 00  |c...............|
00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000880  63 10 a0 01 00 00 00 00  00 00 00 00 00 00 00 00  |c...............|
00000890  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000ff0  00 00 00 00 00 00 00 00  67 30 a0 01 00 00 00 00  |........g0......|
00001000
---

These are basically the PTE entries it gets for the CONFIG_XEN case
which we clear because we're on baremetal.

Now my hunch is that those entries get overwritten later but I wouldn't
want to debug any strange bugs from leftovers in init_level4_pgt so
having the clear_page() is actually a good thing.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/