> On Aug 1, 2018, at 9:52 AM, Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx> wrote:
>
> On 8/1/2018 8:56 AM, Eric Snowberg wrote:
>> Commit d2078d5adbe2 ("x86: EFI stub support for large memory maps")
>> added support for EFI memory maps larger than 128 entries. Following
>> allocation for this new buffer (within the exit_boot function calling
>> alloc_e820ext) another call to efi_get_memory_map was performed.
>> This took place before the final call to ExitBootServices (EBS) to
>> insure the memory map buffer was large enough since memory allocation
>> is not allowed after calling EBS.
>> Later this code was refactored and portions were added to
>> efi-stub-helper.c. The call to efi_get_memory_map did not remain after
>> calling alloc_e820ext. In a later patch, a new EFI_MMAP_NR_SLACK_SLOTS was
>> introduced to help give some headroom should the memory map grow after
>> the EBS is called. This does not alway provide enough space with large
>> memory maps.
> It might be nice if you CC'd the authors of the changes you are "fixing" so that they can chime in. I guess I'm lucky I noticed this on the list. You should probably also have a "fixes tag”.
Sorry I missed you.
>
> Can you elaborate on the specific scenario where EFI_MMAP_NR_SLACK_CLOTS is insufficient? What is the exact failure encountered?
A hard hang.
>
>> This change will call efi_get_memory_map one final time before calling EBS,
>> ensuring there is enough room for the memory map buffer. The additional
>> headroom will be left alone for any memory change that may currently
>> be in flight between the final efi_get_memory_map call and EBS.
>
> Uhh, I'm thinking this should be Nack'd.
>
> This invalidates the entire point of the priv_func because now the memory map may have changed after it was processed by priv_func, but the mem map may not have changed between your added call and EBS. If that happens, EBS will pass, and we'll go into linux with a corrupt mem map.
How would we go into Linux with a corrupt memory map?
> The mem map can still change between your added call and EBS, so you haven't really done anything to address the existing race condition.
This patch is not trying to fix a race condition. The priv_func is exit_boot_func on x86. exit_boot_func can allocate much more memory than the approx 384 bytes set aside for slack space (thru alloc_e820ext). Once EBS is called, there isn’t enough room available for the new memory map on a very large system (do to the memory that was previously allocated in exit_boot_func).
The race condition that exists between priv_func and the call to EBS will be taken care of by the existing code, thru the slack slots.
>
>> Signed-off-by: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
>> ---
>> drivers/firmware/efi/libstub/efi-stub-helper.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> index b018436..3363b4e 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -845,6 +845,14 @@ efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table_arg,
>> if (status != EFI_SUCCESS)
>> goto free_map;
>> + /*
>> + * priv_func could have allocated memory, thus call efi_get_memory_map
>> + * one more time.
>> + */
>> + status = efi_get_memory_map(sys_table_arg, map);
>> + if (status != EFI_SUCCESS)
>> + goto fail;
>> +
>> status = efi_call_early(exit_boot_services, handle, *map->key_ptr);
>> if (status == EFI_INVALID_PARAMETER) {
>
>
> --
> Jeffrey Hugo
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
> --
> 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
