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
| ||
|
Message-ID: <07d101b3-d17a-7781-f05e-96738e6d6848@linux.intel.com> Date: Mon, 27 Nov 2017 10:11:12 -0800 From: Dave Hansen <dave.hansen@...ux.intel.com> To: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org> Cc: Andy Lutomirski <luto@...nel.org>, Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>, Brian Gerst <brgerst@...il.com>, Denys Vlasenko <dvlasenk@...hat.com>, "H. Peter Anvin" <hpa@...or.com>, Josh Poimboeuf <jpoimboe@...hat.com>, Linus Torvalds <torvalds@...ux-foundation.org>, Peter Zijlstra <peterz@...radead.org>, Rik van Riel <riel@...hat.com>, daniel.gruss@...k.tugraz.at, hughd@...gle.com, keescook@...gle.com, linux-mm@...ck.org, michael.schwarz@...k.tugraz.at, moritz.lipp@...k.tugraz.at, richard.fellner@...dent.tugraz.at Subject: Re: [patch V2 1/5] x86/kaiser: Respect disabled CPU features > --- a/arch/x86/include/asm/pgtable_64.h > +++ b/arch/x86/include/asm/pgtable_64.h > @@ -222,7 +222,8 @@ static inline pgd_t kaiser_set_shadow_pg > * wrong CR3 value, userspace will crash > * instead of running. > */ > - pgd.pgd |= _PAGE_NX; > + if (__supported_pte_mask & _PAGE_NX) > + pgd.pgd |= _PAGE_NX; > } Thanks for catching that. It's definitely a bug. Although, practically, it's hard to hit, right? I think everything 64-bit supports NX unless the hypervisor disabled it or something. > } else if (pgd_userspace_access(*pgdp)) { > /* > --- a/arch/x86/mm/kaiser.c > +++ b/arch/x86/mm/kaiser.c > @@ -42,6 +42,8 @@ > > #define KAISER_WALK_ATOMIC 0x1 > > +static pteval_t kaiser_pte_mask __ro_after_init = ~(_PAGE_NX | _PAGE_GLOBAL); Do we need a comment on this, like: /* * The NX and GLOBAL bits are not supported on all CPUs. * We will add them back to this mask at runtime in * kaiser_init_all_pgds() if supported. */ > /* > * At runtime, the only things we map are some things for CPU > * hotplug, and stacks for new processes. No two CPUs will ever > @@ -244,11 +246,14 @@ static pte_t *kaiser_shadow_pagetable_wa > int kaiser_add_user_map(const void *__start_addr, unsigned long size, > unsigned long flags) > { > - pte_t *pte; > unsigned long start_addr = (unsigned long)__start_addr; > unsigned long address = start_addr & PAGE_MASK; > unsigned long end_addr = PAGE_ALIGN(start_addr + size); > unsigned long target_address; > + pte_t *pte; > + > + /* Clear not supported bits */ > + flags &= kaiser_pte_mask; Should we be warning on these if we clear them? Seems kinda funky to silently fix them up. > for (; address < end_addr; address += PAGE_SIZE) { > target_address = get_pa_from_kernel_map(address); > @@ -308,6 +313,11 @@ static void __init kaiser_init_all_pgds( > pgd_t *pgd; > int i; > > + if (__supported_pte_mask & _PAGE_NX) > + kaiser_pte_mask |= _PAGE_NX; > + if (boot_cpu_has(X86_FEATURE_PGE)) > + kaiser_pte_mask |= _PAGE_GLOBAL; Practically, I guess boot_cpu_has(X86_FEATURE_PGE) == (cr4_read() & X86_CR4_PGE). But, in a slow path like this, is it perhaps better to just be checking CR4 directly? Looks functionally fine to me, though. Feel free to add my Reviewed-by or Acked-by.
Powered by blists - more mailing lists