On 20 December 2016 at 12:53, Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 13 Dec, at 10:25:10AM, Ard Biesheuvel wrote:
>> From: Peter Jones <pjones@xxxxxxxxxx>
>>
>> Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
>> (2.28), include memory map entries with phys_addr=0x0 and num_pages=0.
>>
>> Currently the log output for this case (with efi=debug) looks like:
>>
>> [ 0.000000] efi: mem45: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
>>
>> This is clearly wrong, and also not as informative as it could be. This
>> patch changes it so that if we find obviously invalid memory map
>> entries, we print an error and those entries. It also detects the
>> display of the address range calculation overflow, so the new output is:
>>
>> [ 0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
>> [ 0.000000] efi: mem45: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0x0000000000000000] (invalid)
>>
>> It also detects memory map sizes that would overflow the physical
>> address, for example phys_addr=0xfffffffffffff000 and
>> num_pages=0x0200000000000001, and prints:
>>
>> [ 0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
>> [ 0.000000] efi: mem45: [Reserved | | | | | | | | | | | | ] range=[phys_addr=0xfffffffffffff000-0x20ffffffffffffffff] (invalid)
>>
>> It then removes these entries from the memory map.
>>
>> Cc: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Peter Jones <pjones@xxxxxxxxxx>
>> [ardb: refactor for clarity with no functional changes, avoid PAGE_SHIFT]
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> ---
>>
>> I took the liberty of refactoring the code because I found it difficult
>> to understand. No functional changes are intended, although I couldn't
>> figure out if the memcpy() in the original code is (a) correct, and (b)
>> attempts to copy multiple entries at once.
>>
>> Also, pr_warn sounds more appropriate for complaining about broken firmware,
>> but perhaps this is archite-cultural thing as well.
>>
>> arch/x86/platform/efi/efi.c | 70 ++++++++++++++++++++
>> include/linux/efi.h | 1 +
>> 2 files changed, 71 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index bf99aa7005eb..0a1550b82beb 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -210,6 +210,74 @@ int __init efi_memblock_x86_reserve_range(void)
>> return 0;
>> }
>>
>> +#define OVERFLOW_ADDR_SHIFT (64 - EFI_PAGE_SHIFT)
>> +#define OVERFLOW_ADDR_MASK (U64_MAX << OVERFLOW_ADDR_SHIFT)
>> +#define U64_HIGH_BIT (~(U64_MAX >> 1))
>> +
>> +static bool __init efi_memmap_entry_valid(const efi_memory_desc_t *md, int i)
>> +{
>> + static __initdata bool once = true;
>> + u64 end = (md->num_pages << EFI_PAGE_SHIFT) + md->phys_addr - 1;
>> + u64 end_hi = 0;
>> + char buf[64];
>> +
>> + if (md->num_pages == 0) {
>> + end = 0;
>> + } else if (md->num_pages > EFI_PAGES_MAX ||
>> + EFI_PAGES_MAX - md->num_pages <
>> + (md->phys_addr >> EFI_PAGE_SHIFT)) {
>> + end_hi = (md->num_pages & OVERFLOW_ADDR_MASK)
>> + >> OVERFLOW_ADDR_SHIFT;
>> +
>> + if ((md->phys_addr & U64_HIGH_BIT) && !(end & U64_HIGH_BIT))
>> + end_hi += 1;
>> + } else {
>> + return true;
>> + }
>> +
>> + if (once) {
>> + pr_warn(FW_BUG "Invalid EFI memory map entries:\n");
>> + once = false;
>> + }
>
> Maybe use pr_warn_once() here instead of rolling your own 'once' flag?
>
> Otherwise this looks fine to me.
OK, I have pushed this patch to efi/next with the suggested modification.
Peter, what else do we need on top?
--
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
