On Fri, Dec 09, 2016 at 04:18:04PM +0000, Ard Biesheuvel wrote:
> On 7 December 2016 at 21:38, Peter Jones <pjones@xxxxxxxxxx> wrote:
> > 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, say so when we're printing the memory map. 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 entry for 0x0 pages at 0x0000000000000000
> > [ 0.000000] efi: mem45: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0x0000000000000000] (0MB)
> >
> > Signed-off-by: Peter Jones <pjones@xxxxxxxxxx>
> > ---
> > arch/x86/platform/efi/efi.c | 18 +++++++++++++++++-
> > 1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 936a488..0263391 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -217,11 +217,27 @@ void __init efi_print_memmap(void)
> >
> > for_each_efi_memory_desc(md) {
> > char buf[64];
> > + bool valid = true;
> > + u64 size = (md->num_pages << EFI_PAGE_SHIFT) - 1;
> > +
> > + if (md->num_pages == 0) {
> > + size = 0;
> > + valid = false;
> > + }
> > +
>
> This makes sense to me
>
> > + if (md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT)) {
> > + size = (u64)-1LL;
> > + valid = false;
> > + }
> > +
>
> ... but this not so much: setting size to U64_MAX does not do much to
> clarify the line printed below
>
> > + if (!valid)
> > + pr_info(FW_BUG "Invalid EFI memory map entry for 0x%llx pages at 0x%016llx\n",
> > + md->num_pages, md->phys_addr);
> >
> > pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
> > i++, efi_md_typeattr_format(buf, sizeof(buf), md),
> > md->phys_addr,
> > - md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
> > + md->phys_addr + size,
>
> ... given that this wraps around to md->phys_addr -1
>
> > (md->num_pages >> (20 - EFI_PAGE_SHIFT)));
>
> ... and this still uses the bogus num_pages value
That's true, I didn't even try to fix the shorthand version. I've fixed
that to just say (invalid) now instead of trying to print something like
17592186044416MB or whatnot.
> In general, I think it makes sense to disregard entries with num_pages
> == 0, and disregarding values of num_pages that are obviously bogus is
> also fine (although not as meaningful, given the use case), but that
> means we should ignore the whole entry, and refrain from performing
> arithmetic involving any of these values, or 'sanitize' the size by
> setting it to U64_MAX
I've rewritten the first patch with this in mind - now I print the
warning and cull them from the list. It also does that separately from
efi_print_memmap(), so it still fires even when efi=debug isn't true.
Though given the first patch, it's not clear that the second or fourth
one are really needed any more. They'd only protect against bad entries
we've added ourselves. But I've left them intact until we've discussed
this further.
One question - we should probably be doing this on non-x86 as well.
What's the best way to do that?
All that said, this patchset lets the machine in front of me boot.
--
Peter
--
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
