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
| ||
|
Date: Sun, 30 Oct 2016 09:21:29 -0700 From: Andy Lutomirski <luto@...capital.net> To: Matt Fleming <matt@...eblueprint.co.uk> Cc: "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>, Brian Gerst <brgerst@...il.com>, "linux-tip-commits@...r.kernel.org" <linux-tip-commits@...r.kernel.org>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...nel.org>, Linus Torvalds <torvalds@...ux-foundation.org>, Ard Biesheuvel <ard.biesheuvel@...aro.org>, Josh Poimboeuf <jpoimboe@...hat.com>, Denys Vlasenko <dvlasenk@...hat.com>, "H. Peter Anvin" <hpa@...or.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Nadav Amit <nadav.amit@...il.com>, Borislav Petkov <bp@...en8.de>, Peter Zijlstra <peterz@...radead.org> Subject: Re: [tip:x86/asm] x86/mm/64: Enable vmapped stacks (CONFIG_HAVE_ARCH_VMAP_STACK=y) On Mon, Oct 24, 2016 at 6:09 AM, Matt Fleming <matt@...eblueprint.co.uk> wrote: > > From d2c17f46686076677da3bf04caa2f69d654f8d8a Mon Sep 17 00:00:00 2001 > From: Matt Fleming <matt@...eblueprint.co.uk> > Date: Thu, 20 Oct 2016 22:17:21 +0100 > Subject: [PATCH] x86/efi: Prevent mixed mode boot corruption with > CONFIG_VMAP_STACK > > Booting an EFI mixed mode kernel has been crashing since commit: > Looks reasonable. It's certainly a considerable improvement over the status quo. Minor comments below, mainly of the form "do you need all these changes": > int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages) > { > unsigned long pfn, text; > struct page *page; > unsigned npages; > + void *addr; > pgd_t *pgd; > > if (efi_enabled(EFI_OLD_MEMMAP)) > @@ -251,7 +277,8 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages) > if (!page) > panic("Unable to allocate EFI runtime stack < 4GB\n"); > > - efi_scratch.phys_stack = virt_to_phys(page_address(page)); > + addr = page_address(page); > + efi_scratch.phys_stack = virt_to_phys_or_null_size(addr, PAGE_SIZE); This can't be on the stack -- you just allocated it with alloc_page(). > efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */ > > npages = (_etext - _text) >> PAGE_SHIFT; > @@ -494,8 +521,8 @@ static efi_status_t efi_thunk_get_time(efi_time_t *tm, efi_time_cap_t *tc) > > spin_lock(&rtc_lock); > > - phys_tm = virt_to_phys(tm); > - phys_tc = virt_to_phys(tc); > + phys_tm = virt_to_phys_or_null(tm); > + phys_tc = virt_to_phys_or_null(tc); Seems okay. > @@ -511,7 +538,7 @@ static efi_status_t efi_thunk_set_time(efi_time_t *tm) > > spin_lock(&rtc_lock); > > - phys_tm = virt_to_phys(tm); > + phys_tm = virt_to_phys_or_null(tm); Seems okay. > @@ -529,9 +556,9 @@ efi_thunk_get_wakeup_time(efi_bool_t *enabled, efi_bool_t *pending, > > spin_lock(&rtc_lock); > > - phys_enabled = virt_to_phys(enabled); > - phys_pending = virt_to_phys(pending); > - phys_tm = virt_to_phys(tm); > + phys_enabled = virt_to_phys_or_null(enabled); > + phys_pending = virt_to_phys_or_null(pending); > + phys_tm = virt_to_phys_or_null(tm); All seem okay. > > status = efi_thunk(get_wakeup_time, phys_enabled, > phys_pending, phys_tm); > @@ -549,7 +576,7 @@ efi_thunk_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) > > spin_lock(&rtc_lock); > > - phys_tm = virt_to_phys(tm); > + phys_tm = virt_to_phys_or_null(tm); Seems reasonable. > +static unsigned long efi_name_size(efi_char16_t *name) > +{ > + return ucs2_strsize(name, EFI_VAR_NAME_LEN) + 1; > +} If this is really dynamic, I'm surprised that anything ends up aligned. Can't this be a number like 6? It might pay to extend that warning to also check that, if the variable is on the stack, its size is a power of two. But maybe none of the users of this are on the stack, in which case it might pay to try to prevent a new user on the stack from showing up.
Powered by blists - more mailing lists