Re: [PATCH v2 4/6] efi: memmap: Disregard bogus entries instead of returning them

[Ard Biesheuvel <ardb@xxxxxxxxxx>]

On Mon, 3 Oct 2022 at 17:18, Demi Marie Obenour
<demi@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Oct 03, 2022 at 01:26:23PM +0200, Ard Biesheuvel wrote:
> > The ESRT code currently contains some sanity checks on the memory
> > descriptor it obtains, but these can only trigger when the descriptor is
> > invalid (if at all).
> >
> > So let's drop these checks, and instead, disregard descriptors entirely
> > if the start address is misaligned, or the number of pages reaches
> > beyond the end of the address space. Note that the memory map as a whole
> > could still be inconsistent, i.e., multiple entries might cover the same
> > area, or the address could be outside of the addressable VA space, but
> > validating that goes beyond the scope of these helpers.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > ---
> >  drivers/firmware/efi/efi.c  | 13 +++++++------
> >  drivers/firmware/efi/esrt.c | 18 +-----------------
> >  2 files changed, 8 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 11857af72859..55bd3f4aab28 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, 
> > efi_memory_desc_t *out_md)
> >       efi_memory_desc_t *md;
> >
> >       if (!efi_enabled(EFI_MEMMAP)) {
> > -             pr_err_once("EFI_MEMMAP is not enabled.\n");
> > +             pr_warn_once("EFI_MEMMAP is not enabled.\n");
> >               return -EINVAL;
> >       }
> >
> > -     if (!out_md) {
> > -             pr_err_once("out_md is null.\n");
> > -             return -EINVAL;
> > -        }
> > -
>
> Nit: this seems unrelated.
>
> >       for_each_efi_memory_desc(md) {
> >               u64 size;
> >               u64 end;
> >
> > +             /* skip bogus entries */
> > +             if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
> > +                 (md->phys_addr > 0 &&
> > +                  (md->num_pages > (U64_MAX - md->phys_addr + 1) >> 
> > EFI_PAGE_SHIFT)))
> > +                     continue;
>
> Should this also check if md->num_pages is 0?

Yes, probably.

>  Also, should this check
> be part of for_each_efi_memory_desc()?
>

No, I don't think so. The for_each_xxx() helpers we have throughout
the kernel usually don't incorporate such checks, and I'd prefer to
adhere to the principle of least surprise here.

> > +
> >               size = md->num_pages << EFI_PAGE_SHIFT;
> >               end = md->phys_addr + size;
> >               if (phys_addr >= md->phys_addr && phys_addr < end) {
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index 2a2f52b017e7..8f86f2b0734b 100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -247,9 +247,6 @@ void __init efi_esrt_init(void)
> >       int rc;
> >       phys_addr_t end;
> >
> > -     if (!efi_enabled(EFI_MEMMAP))
> > -             return;
> > -
> >       pr_debug("esrt-init: loading.\n");
> >       if (!esrt_table_exists())
> >               return;
> > @@ -263,21 +260,8 @@ void __init efi_esrt_init(void)
> >               return;
> >       }
> >
> > -     max = efi_mem_desc_end(&md);
> > -     if (max < efi.esrt) {
> > -             pr_err("EFI memory descriptor is invalid. (esrt: %p max: 
> > %p)\n",
> > -                    (void *)efi.esrt, (void *)max);
> > -             return;
> > -     }
> > -
> > +     max = efi_mem_desc_end(&md) - efi.esrt;
> >       size = sizeof(*esrt);
> > -     max -= efi.esrt;
> > -
> > -     if (max < size) {
> > -             pr_err("ESRT header doesn't fit on single memory map entry. 
> > (size: %zu max: %zu)\n",
> > -                    size, max);
> > -             return;
> > -     }
>
> This can still happen if the ESRT pointer is very very close to the end
> of a memory map entry, unless there is another check that handles
> such cases.

You're right - I missed that.