On Mon, Oct 24, 2016 at 6:09 AM, Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
>
> From d2c17f46686076677da3bf04caa2f69d654f8d8a Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
