Re: [PATCHv13 4/9] x86/boot/compressed: Handle unaccepted memory

[Ard Biesheuvel]

On Fri, 2 Jun 2023 at 17:37, Kirill A. Shutemov
<kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Jun 02, 2023 at 04:06:41PM +0200, Borislav Petkov wrote:
> > On Thu, Jun 01, 2023 at 09:25:38PM +0300, Kirill A. Shutemov wrote:
> > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > > index 454757fbdfe5..749f0fe7e446 100644
> > > --- a/arch/x86/boot/compressed/kaslr.c
> > > +++ b/arch/x86/boot/compressed/kaslr.c
> > > @@ -672,6 +672,28 @@ static bool process_mem_region(struct mem_vector *region,
> > >  }
> > >
> > >  #ifdef CONFIG_EFI
> > > +
> > > +/*
> > > + * Only EFI_CONVENTIONAL_MEMORY and EFI_UNACCEPTED_MEMORY (if supported) are
> > > + * guaranteed to be free.
> > > + *
> > > + * It is more conservative in picking free memory than the EFI spec allows:
> >
> > "Pick free memory more conservatively than the EFI spec allows:
> > EFI_BOOT_SERVICES_ ..."
>
> Okay.
>
> > > + *
> > > + * According to the spec, EFI_BOOT_SERVICES_{CODE|DATA} are also free memory
> > > + * and thus available to place the kernel image into, but in practice there's
> > > + * firmware where using that memory leads to crashes.
> >
> > ... because that firmware still scratches into that memory or why?
>
> I moved the existing comment. I don't have have any context beyond that.
>
> Relevant commit: 0982adc74673 ("x86/boot/KASLR: Work around firmware bugs
> by excluding EFI_BOOT_SERVICES_* and EFI_LOADER_* from KASLR's choice")
>
> Ard, do you have anything to add here?
>

The problem is that on x86, there is buggy vendor/OEM EFI code that
registers for internal events that trigger when SetVirtualAddressMap()
is called, and assume that at that point, EfiBootServicesData memory
regions have not been touched by the loader yet, which is probably
true if you are booting Windows.

So on x86, the kernel proper also preserves these regions until after
it calls SetVirtualAddressMap() (efi_free_boot_services() in
arch/x86/platform/efi/quirks.c)

So for the same reason, this code needs to disregard those regions as well.


> > > + */
> > > +static inline bool memory_type_is_free(efi_memory_desc_t *md)
> > > +{
> > > +   if (md->type == EFI_CONVENTIONAL_MEMORY)
> > > +           return true;
> > > +
> > > +   if (md->type == EFI_UNACCEPTED_MEMORY)
> > > +           return IS_ENABLED(CONFIG_UNACCEPTED_MEMORY);
> >
> > Make it plan and simple:
> >
> >       if (IS_ENABLED(CONFIG_UNACCEPTED_MEMORY) &&
> >           md->type == EFI_UNACCEPTED_MEMORY)
> >               return true;
>
> I don't see why it is simpler. It looks unnecessary noisy to me.
>
> But okay.
>
> > > +
> > > +   return false;
> > > +}
> > > +
> > >  /*
> > >   * Returns true if we processed the EFI memmap, which we prefer over the E820
> > >   * table if it is available.
> > > @@ -716,18 +738,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
> > >     for (i = 0; i < nr_desc; i++) {
> > >             md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
> > >
> > > -           /*
> > > -            * Here we are more conservative in picking free memory than
> > > -            * the EFI spec allows:
> > > -            *
> > > -            * According to the spec, EFI_BOOT_SERVICES_{CODE|DATA} are also
> > > -            * free memory and thus available to place the kernel image into,
> > > -            * but in practice there's firmware where using that memory leads
> > > -            * to crashes.
> > > -            *
> > > -            * Only EFI_CONVENTIONAL_MEMORY is guaranteed to be free.
> > > -            */
> > > -           if (md->type != EFI_CONVENTIONAL_MEMORY)
> > > +           if (!memory_type_is_free(md))
> > >                     continue;
> > >
> > >             if (efi_soft_reserve_enabled() &&
> > > diff --git a/arch/x86/boot/compressed/mem.c b/arch/x86/boot/compressed/mem.c
> > > index 67594fcb11d9..4ecf26576a77 100644
> > > --- a/arch/x86/boot/compressed/mem.c
> > > +++ b/arch/x86/boot/compressed/mem.c
> > > @@ -1,9 +1,40 @@
> > >  // SPDX-License-Identifier: GPL-2.0-only
> > >
> > >  #include "error.h"
> > > +#include "misc.h"
> > >
> > >  void arch_accept_memory(phys_addr_t start, phys_addr_t end)
> > >  {
> > >     /* Platform-specific memory-acceptance call goes here */
> > >     error("Cannot accept memory");
> > >  }
> > > +
> > > +void init_unaccepted_memory(void)
> > > +{
> > > +   guid_t guid =  LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID;
> >
> > An additional space after the "=".
>
> Okay.
>
> > > +   struct efi_unaccepted_memory *unaccepted_table;
> > > +   unsigned long cfg_table_pa;
> > > +   unsigned int cfg_table_len;
> > > +   enum efi_type et;
> > > +   int ret;
> > > +
> > > +   et = efi_get_type(boot_params);
> > > +   if (et == EFI_TYPE_NONE)
> > > +           return;
> > > +
> > > +   ret = efi_get_conf_table(boot_params, &cfg_table_pa, &cfg_table_len);
> > > +   if (ret)
> > > +           error("EFI config table not found.");
> >
> > What's the point in erroring out here?
>
> Configuration table suppose to be present, even if unaccepted memory is
> not supported. Something is very wrong if it is missing.
>
> I will downgrade it warn().
>
> > > +   unaccepted_table = (void *)efi_find_vendor_table(boot_params,
> > > +                                                    cfg_table_pa,
> > > +                                                    cfg_table_len,
> > > +                                                    guid);
> > > +   if (!unaccepted_table)
> > > +           return;
> > > +
> > > +   if (unaccepted_table->version != 1)
> > > +           error("Unknown version of unaccepted memory table\n");
> > > +
> > > +   set_unaccepted_table(unaccepted_table);
> >
> > Why is this a function at all and not a simple assignment?
>
> I wanted to keep unaccepted_table private to the libstub/unaccepted_memory.c.
> The setter provides a good spot for documentation to guide unaccepted
> memory enablers for other archs.
>
> Still want replace it with direct assignment?
>
> >
> > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > > index 014ff222bf4b..36535a3753f5 100644
> > > --- a/arch/x86/boot/compressed/misc.c
> > > +++ b/arch/x86/boot/compressed/misc.c
> > > @@ -455,6 +455,13 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> > >  #endif
> > >
> > >     debug_putstr("\nDecompressing Linux... ");
> > > +
> > > +   if (IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) {
> > > +           debug_putstr("Accepting memory... ");
> >
> > This needs to happen...
> >
> > > +           init_unaccepted_memory();
> >
> > ... after the init, after the init has parsed the config table and has
> > found unaccepted memory.
> >
> > If not, you don't need to issue anything as that would be wrong.
>
> Okay, I will make init_unaccepted_memory() return true if unaccepted
> memory is present and hide defined it always-false for !UNACCEPTED_MEMORY.
> So this hunk will look this way:
>
>         if (init_unaccepted_memory()) {
>                 debug_putstr("Accepting memory... ");
>                 accept_memory(__pa(output), __pa(output) + needed_size);
>         }
>
> --
>   Kiryl Shutsemau / Kirill A. Shutemov