Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Dave Young]

On 06/08/17 at 10:02am, Ard Biesheuvel wrote:
> On 8 June 2017 at 05:32, Dave Young <dyoung@xxxxxxxxxx> wrote:
> > Maniaxx <tripleshiftone@xxxxxxxxx> reported kernel boot panic similar to below:
> > (emulated the panic with using same invalid phys addr in a uefi vm)
> > There are also a bug in bugzilla.kernel.org:
> > https://bugzilla.kernel.org/show_bug.cgi?id=195633
> >
> > This happens after below commit:
> > 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> >
> > The root cause is the firmware on those machines provides invalid bgrt
> > image addresses.
> >
> > With original efi bgrt code we initialize bgrt late
> > and use ioremap to map the image address. In ioremap code we check the
> > address is a valid physical address or not before really map it.
> >
> > With current new efi bgrt code we moved the initialization to early code
> > so we switch to early_memremap which does not check the phys_addr like
> > ioremap does. This lead to the early kernel panics.
> >
> > Fix this by checking the image physical address, if it is not within
> > any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
> > then the original ioremap checking, according to spec the BGRT data
> > should fall into EFI_BOOT_SERVICES_DATA.
> >
> 
> Which spec? The UEFI spec does not mention BGRT, and given that it is

It is mentioned in ACPI spec, see 6.1 spec section 5.2.22.4
http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf

> an ACPI table, I would expect an ACPI reclaim region to be the most
> appropriate. A quick test with QEMU confirms this:
> 
> ACPI: BGRT 0x000000013A5E0000 000038 (v01 INTEL  EDK2     00000002
>  01000013)
> 
> and
> 
> efi:   0x00013a5e0000-0x00013a5effff [ACPI Reclaim Memory|   |  |  |
> |  |  |  |   |WB|  |  |  ]
> 
> So while I agree that we have to fix this, and that checking the BGRT
> address against the UEFI memory map is the most appropriate course of
> action, requiring a certain region type is probably not what we want.
> 
> We have a similar check for ESRT, in efi_mem_desc_lookup(), which
> looks a bit dodgy tbh, given that it allows any region type (including
> MMIO), as long as it has the EFI_MEMORY_RUNTIME attribute, which is
> almost certainly incorrect.

Yes, I had that in mind but it delayed for something else ..

> 
> So what I would like to see is a function that can tell you whether a
> certain address is covered by a region of a type that is normal
> memory, and is occupied, i.e.,
> 
> EFI_RESERVED_TYPE
> EFI_LOADER_CODE
> EFI_LOADER_DATA
> EFI_BOOT_SERVICES_CODE
> EFI_BOOT_SERVICES_DATA
> EFI_RUNTIME_SERVICES_CODE
> EFI_RUNTIME_SERVICES_DATA
> EFI_ACPI_RECLAIM_MEMORY
> EFI_ACPI_MEMORY_NVS
> 
> The EFI_MEMORY_RUNTIME attribute is irrelevant: the firmware itself
> does not have to read these tables at runtime, so it doesn't matter
> whether the O/S maps them on its behalf.
> 
> If you could please stick that in drivers/firmware/efi/efi.c, and
> rework the patch to use it instead? I will move the ESRT code to it as
> well once this is merged.

Will think about this, I had some plan to change the desc lookup
function but it was delayed for something else. For this bgrt issue since
acpi spec clearly said it is in boot data, can we just check the boot
data areas?

Thanks
Dave