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

[Kirill A. Shutemov]

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?

> > + */
> > +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