Re: [PATCH v2 1/2] efi/libstub: arm64: Force Image reallocation if BSS was not reserved

["Herrenschmidt, Benjamin" <benh@amazon.com>]

Looks good but I'll try to actually test it on my repro-setup tomorrow.

Cheers,
Ben.

> On Mon, 2021-07-26 at 12:09 +0200, Ard Biesheuvel wrote:
> Distro versions of GRUB replace the usual LoadImage/StartImage calls
> used to load the kernel image with some local code that fails to
> honor
> the allocation requirements described in the PE/COFF header, as it
> does not account for the image's BSS section at all: it fails to
> allocate space for it, and fails to zero initialize it.
> 
> Since the EFI stub itself is allocated in the .init segment, which is
> in the middle of the image, its BSS section is not impacted by this,
> and the main consequence of this omission is that the BSS section may
> overlap with memory regions that are already used by the firmware.
> 
> So let's warn about this condition, and force image reallocation to
> occur in this case, which works around the problem.
> 
> Fixes: 82046702e288 ("efi/libstub/arm64: Replace 'preferred' offset
> with alignment check")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 49 +++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c
> b/drivers/firmware/efi/libstub/arm64-stub.c
> index 7bf0a7acae5e..3698c1ce2940 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -34,6 +34,51 @@ efi_status_t check_platform_features(void)
>  	return EFI_SUCCESS;
>  }
>  
> +/*
> + * Distro versions of GRUB may ignore the BSS allocation entirely
> (i.e., fail
> + * to provide space, and fail to zero it). Check for this condition
> by double
> + * checking that the first and the last byte of the image are
> covered by the
> + * same EFI memory map entry.
> + */
> +static bool check_image_region(u64 base, u64 size)
> +{
> +	unsigned long map_size, desc_size, buff_size;
> +	efi_memory_desc_t *memory_map;
> +	struct efi_boot_memmap map;
> +	efi_status_t status;
> +	bool ret = false;
> +	int map_offset;
> +
> +	map.map =	&memory_map;
> +	map.map_size =	&map_size;
> +	map.desc_size =	&desc_size;
> +	map.desc_ver =	NULL;
> +	map.key_ptr =	NULL;
> +	map.buff_size =	&buff_size;
> +
> +	status = efi_get_memory_map(&map);
> +	if (status != EFI_SUCCESS)
> +		return false;
> +
> +	for (map_offset = 0; map_offset < map_size; map_offset +=
> desc_size) {
> +		efi_memory_desc_t *md = (void *)memory_map +
> map_offset;
> +		u64 end = md->phys_addr + md->num_pages *
> EFI_PAGE_SIZE;
> +
> +		/*
> +		 * Find the region that covers base, and return whether
> +		 * it covers base+size bytes.
> +		 */
> +		if (base >= md->phys_addr && base < end) {
> +			ret = (base + size) <= end;
> +			break;
> +		}
> +	}
> +
> +	efi_bs_call(free_pool, memory_map);
> +
> +	return ret;
> +}
> +
>  /*
>   * Although relocatable kernels can fix up the misalignment with
> respect to
>   * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly
> out of
> @@ -92,7 +137,9 @@ efi_status_t handle_kernel_image(unsigned long
> *image_addr,
>  	}
>  
>  	if (status != EFI_SUCCESS) {
> -		if (IS_ALIGNED((u64)_text, min_kimg_align())) {
> +		if (!check_image_region((u64)_text, kernel_memsize)) {
> +			efi_err("FIRMWARE BUG: Image BSS overlaps
> adjacent EFI memory region\n");
> +		} else if (IS_ALIGNED((u64)_text, min_kimg_align())) {
>  			/*
>  			 * Just execute from wherever we were loaded by
> the
>  			 * UEFI PE/COFF loader if the alignment is
> suitable.