lkml.org 
[lkml]   [2017]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address
On 8 June 2017 at 14:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 June 2017 at 14:20, Dave Young <dyoung@redhat.com> wrote:
>> On 06/08/17 at 10:02am, Ard Biesheuvel wrote:
>>> On 8 June 2017 at 05:32, Dave Young <dyoung@redhat.com> wrote:
>>> > Maniaxx <tripleshiftone@gmail.com> 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.

Oops.

So you are validating the image address not the table address.
Apologies for taking some time to catch up :-)

\
 
 \ /
  Last update: 2017-06-12 01:30    [W:0.566 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site