On 8 January 2015 at 19:04, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> Hi Ard,
>
> On Thu, Jan 08, 2015 at 05:51:47PM +0000, Ard Biesheuvel wrote:
>> This fixes two minor issues in the implementation of get_memory_map():
>> - Currently, it assumes that sizeof(efi_memory_desc_t) == desc_size,
>> which is usually true, but not mandated by the spec. (This was added
>> intentionally to allow future additions to the definition of
>> efi_memory_desc_t). The way the loop is implemented currently, the
>> added slack space may be insufficient if desc_size is larger, which in
>> some corner cases could result in the loop never terminating.
>> - It allocates 32 efi_memory_desc_t entries first (again, using the size
>> of the struct instead of desc_size), and frees and reallocates if it
>> turns out to be insufficient. Few implementations of UEFI have such small
>> memory maps, which results in a unnecessary allocate/free pair on each
>> invocation.
>>
>> Fix this by calling the get_memory_map() boot service first with a '0'
>> input value for map size to retrieve the map size and desc size from the
>> firmware and only then perform the allocation, using desc_size rather
>> than sizeof(efi_memory_desc_t).
>
> Is the desc_size guaranteed to be set up correctly if the size is too
> small?
>
> As far as I can see, for that case the spec only mandates that
> MemoryMapSize is updated nd EFI_BUFFER_TOO_SMALL is returned.
>
> It's not clear to me whether DescriptorSize or DescriptorVersion are
> initialised in cases other than success.
>
The way I read it, descriptor size and descriptor version are always
returned, e.g., as opposed to MapKey, which is only returned on
success, and the spec mentions that specifically.
We could ask for clarification just to be sure.
--
Ard.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> ---
>> drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> index e766df60fbfb..caf91eab0bfc 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -75,25 +75,29 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
>> unsigned long key;
>> u32 desc_version;
>>
>> - *map_size = sizeof(*m) * 32;
>> -again:
>> + *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;
>> +
>> /*
>> * Add an additional efi_memory_desc_t because we're doing an
>> * allocation which may be in a new descriptor region.
>> */
>> - *map_size += sizeof(*m);
>> + *map_size += *desc_size;
>> 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);
>> - goto again;
>> + return EFI_LOAD_ERROR;
>> }
>>
>> if (status != EFI_SUCCESS)
>> --
>> 1.8.3.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
