Re: kexec regression since 4.9 caused by efi

[Ard Biesheuvel]

On 21 March 2017 at 07:48, Dave Young <dyoung@xxxxxxxxxx> wrote:
> On 03/20/17 at 10:14am, Dave Young wrote:
>> On 03/17/17 at 01:32pm, Matt Fleming wrote:
>> > On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote:
>> > >
>> > > Matt, I think it should be fine although I think the md type checking in
>> > > efi_mem_desc_lookup() is causing confusion and not easy to understand..
>> >
>> > Could you make that a separate patch if you think of improvements
>> > there?
>>
>> Duplicate the lookup function is indeed a little ugly, will do it when I
>> have a better idea, we can leave it as is since it works.
>
> Matt, rethinking about this, how about doint something below, not
> tested, just seeking for idea and opinons, in this way no need duplicate
> a function, but there is an assumption that no overlapped mem ranges in
> efi memmap.
>
> Also the case Omar reported is the esrt memory range type is
> RUNTIME_DATA, that is a little different with the mem attribute of
> RUNTIME which also includes BOOT_DATA which has been set the RUNTIME
> attribute, like bgrt in kexec reboot. Should we distinguish the two
> cases and give out some warnings or debug info?
>
>
> ---
>  arch/x86/platform/efi/quirks.c |    5 +++++
>  drivers/firmware/efi/efi.c     |    6 ------
>  drivers/firmware/efi/esrt.c    |    7 +++++++
>  3 files changed, 12 insertions(+), 6 deletions(-)
>
> --- linux-x86.orig/drivers/firmware/efi/efi.c
> +++ linux-x86/drivers/firmware/efi/efi.c
> @@ -376,12 +376,6 @@ int __init efi_mem_desc_lookup(u64 phys_
>                 u64 size;
>                 u64 end;
>
> -               if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> -                   md->type != EFI_BOOT_SERVICES_DATA &&
> -                   md->type != EFI_RUNTIME_SERVICES_DATA) {
> -                       continue;
> -               }
> -
>                 size = md->num_pages << EFI_PAGE_SHIFT;
>                 end = md->phys_addr + size;
>                 if (phys_addr >= md->phys_addr && phys_addr < end) {
> --- linux-x86.orig/drivers/firmware/efi/esrt.c
> +++ linux-x86/drivers/firmware/efi/esrt.c
> @@ -258,6 +258,13 @@ void __init efi_esrt_init(void)
>                 return;
>         }
>
> +       if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> +             md->type != EFI_BOOT_SERVICES_DATA &&
> +             md->type != EFI_RUNTIME_SERVICES_DATA) {
> +               pr_err("ESRT header memory map type is invalid\n");
> +               return;
> +       }
> +

This looks wrong to me. While the meanings get convoluted in practice,
the EFI_MEMORY_RUNTIME attribute only means that the firmware requests
a virtual mapping for the region. It is perfectly legal for a
EFI_RUNTIME_SERVICES_DATA region not to have the EFI_MEMORY_RUNTIME
attribute, if the region is never accessed by the runtime services
themselves, and this is not entirely unlikely for tables that the
firmware exposes to the OS

>         max = efi_mem_desc_end(&md);
>         if (max < efi.esrt) {
>                 pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -201,6 +201,11 @@ void __init efi_arch_mem_reserve(phys_ad
>                 return;
>         }
>
> +       if (md->attribute & EFI_MEMORY_RUNTIME ||
> +             md->type != EFI_BOOT_SERVICES_DATA) {
> +               return;
> +       }
> +
>         size += addr % EFI_PAGE_SIZE;
>         size = round_up(size, EFI_PAGE_SIZE);
>         addr = round_down(addr, EFI_PAGE_SIZE);
>
>>
>> >
>> > > How about move the if chunk early like below because it seems no need
>> > > to sanity check the addr + size any more if the md is still RUNTIME?
>> >
>> > My original version did as you suggest, but I changed it because we
>> > *really* want to know if someone tries to reserve a range that spans
>> > regions. That would be totally unexpected and a warning about a
>> > potential bug/issue.
>>
>> Matt, I'm fine if you prefer to capture the range checking errors.
>> Would you like me to post it or just you send it out?
>>
>> Thanks
>> Dave
>
> Thanks
> Dave
> --
> 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