[PATCH] efi: get_memory_map: add sufficient slack for memory descriptors

[Ard Biesheuvel]

> On 14 Feb 2015, at 00:04, Matt Fleming <matt at codeblueprint.co.uk> wrote:
> 
>> On Thu, 12 Feb, at 11:31:02PM, Ard Biesheuvel wrote:
>> 
>> Actually, looking again at the original patch, it appears that my
>> analysis was incorrect regarding the possibility that the loop would
>> never terminate. The only thing that could happen if desc_size >
>> sizeof(efi_memory_desc_t) is that you need two iterations instead of
>> one to get a pool allocation that is of sufficient size.
>> So perhaps it is better to just revert the patch.
>> 
>> My apologies for the hassle.
> 
> This is what I've got queued up,
> 

Acked-by: Ard Biesheuvel <ard at linaro.org>

Thanks Matt

> ---
> 
> From 3f281b98ffc99e604a3988aa93304a3a591eeeb5 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming at intel.com>
> Date: Fri, 13 Feb 2015 15:46:56 +0000
> Subject: [PATCH] Revert "efi/libstub: Call get_memory_map() to obtain map and
> desc sizes"
> 
> This reverts commit d1a8d66b9177105e898e73716f97eb61842c457a.
> 
> Ard reported a boot failure when running UEFI under Qemu and Xen and
> experimenting with various Tianocore build options,
> 
> "As it turns out, when allocating room for the UEFI memory map using
>  UEFI's AllocatePool (), it may result in two new memory map entries
>  being created, for instance, when using Tianocore's preallocated region
>  feature. For example, the following region
> 
>  0x00005ead5000-0x00005ebfffff [Conventional Memory|   |  |  |  |  |WB|WT|WC|UC]
> 
>  may be split like this
> 
>  0x00005ead5000-0x00005eae2fff [Conventional Memory|   |  |  |  |  |WB|WT|WC|UC]
>  0x00005eae3000-0x00005eae4fff [Loader Data        |   |  |  |  |  |WB|WT|WC|UC]
>  0x00005eae5000-0x00005ebfffff [Conventional Memory|   |  |  |  |  |WB|WT|WC|UC]
> 
>  if the preallocated Loader Data region was chosen to be right in the
>  middle of the original free space.
> 
>  After patch d1a8d66b9177 ("efi/libstub: Call get_memory_map() to
>  obtain map and desc sizes"), this is not being dealt with correctly
>  anymore, as the existing logic to allocate room for a single additional
>  entry has become insufficient."
> 
> Mark requested to reinstate the old loop we had before commit
> d1a8d66b9177, which grows the memory map buffer until it's big enough to
> hold the EFI memory map.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Mark Rutland <mark.rutland at arm.com>
> Signed-off-by: Matt Fleming <matt.fleming at intel.com>
> ---
> drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index d073e3946383..9bd9fbb5bea8 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -66,29 +66,25 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
>    unsigned long key;
>    u32 desc_version;
> 
> -    *map_size = 0;
> -    *desc_size = 0;
> -    key = 0;
> -    status = efi_call_early(get_memory_map, map_size, NULL,
> -                &key, desc_size, &desc_version);
> -    if (status != EFI_BUFFER_TOO_SMALL)
> -        return EFI_LOAD_ERROR;
> -
> +    *map_size = sizeof(*m) * 32;
> +again:
>    /*
>     * Add an additional efi_memory_desc_t because we're doing an
>     * allocation which may be in a new descriptor region.
>     */
> -    *map_size += *desc_size;
> +    *map_size += sizeof(*m);
>    status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>                *map_size, (void **)&m);
>    if (status != EFI_SUCCESS)
>        goto fail;
> 
> +    *desc_size = 0;
> +    key = 0;
>    status = efi_call_early(get_memory_map, map_size, m,
>                &key, desc_size, &desc_version);
>    if (status == EFI_BUFFER_TOO_SMALL) {
>        efi_call_early(free_pool, m);
> -        return EFI_LOAD_ERROR;
> +        goto again;
>    }
> 
>    if (status != EFI_SUCCESS)
> -- 
> 1.9.3
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center