On 8 June 2017 at 14:20, Dave Young <dyoung@xxxxxxxxxx> wrote: > 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 > I understand that. The reason for asking 'which spec' was your claim that 'according to spec the BGRT data should fall into EFI_BOOT_SERVICES_DATA' >> 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? > The BGRT *table* does not reside in EFI_BOOT_SERVICES_DATA, but usually in ACPI reclaim memory. The BGRT *image* it points to must reside in EFI_BOOT_SERVICES_DATA according to the ACPI spec, but this region is disjoint from the ACPI table. -- 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
