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.
--
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
