[RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Jeff Hugo]

From: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

There exists a race condition between when the efi stub grabs the memory
map, and when ExitBootServices is called at which point the EFI can process
an event which causes the memory map to be invalidated.  According to the
UEFI spec, ExitBootServices will return EFI_INVALID_PARAMETER if this
occurs, at which point the efi stub is expected to obtain the new memory
map, and retry the call to ExitBootServices.

Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---

I'm not particularly happy with the current state of this fix, but I feel like
this issue is somewhat unsolvable.  Currently I've based this solution upon
arch/x86/boot/compressed/eboot.c however it has two primary issues I'd like
feedback upon-

First issue-
efi_get_memory_map() performs an allocation as it attempts to create a
minimal sized buffer to hold the map.  Per my understanding of the UEFI spec,
allocations are not permitted after a call to ExitBootServices:

"A UEFI OS loader should not make calls to any boot service function other than
GetMemoryMap() after the first call to ExitBootServices()."

However the only alternative I can think of it to allocate a sufficently large
buffer so that it can be reused to hold the modified memory map.  There doesn't
seem to be any limit to the new map, so any buffer space value I would choose
would be arbitrary, and there would be some chance that it would be insufficent.
efi_get_memory_map() would need to be modified to "return" the origional size
of the allocated buffer as well, so I feel like this solution makes a mess of
the code, reduces the value of the efi_get_memory_map() helper function, and for
all that effort, we still can't fully address this race condition.

I guess the question is, where do we draw the line at "good enough" for
addressing this issue?  Do we use efi_get_memory_map() since it appears to be
cleaner and does not seem to cause a problem today, despite violating the spec?

Second issue-
When do we give up if we cannot get a good memory map to use with
ExitBootServices?  Currently there is an infinite loop in my patch.  I noticed
arch/x86/boot/compressed/eboot.c only retrys after the first failure.  I think
a single retry is insufficent, we could do better, but I'm aware that an
infinite loop is generally a bad idea.  Any strong opinions on when to quit?
100 attempts?  Either way, it seems the system will require a hard reboot if
the retry(s) ever end up failing.

 drivers/firmware/efi/libstub/fdt.c | 45 ++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index e58abfa..d3e70b0 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -250,16 +250,43 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		}
 	}
 
-	/*
-	 * Update the memory map with virtual addresses. The function will also
-	 * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
-	 * entries so that we can pass it straight into SetVirtualAddressMap()
-	 */
-	efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
-			&runtime_entry_count);
+	do {
+		/*
+		 * We should not free and/or allocate after calling
+		 * ExitBootServices, but this avoids the issue of guessing how
+		 * large of a buffer we need to store an updated memory map,
+		 * and its common for the implementation of ExitBootServices to
+		 * first check if the map key is current, and error out early
+		 * leaving all other services active.
+		 */
+		sys_table->boottime->free_pool(memory_map);
+		status = efi_get_memory_map(sys_table, &memory_map, &map_size,
+					    &desc_size, &desc_ver, &mmap_key);
+		if (status != EFI_SUCCESS) {
+			pr_efi_err(sys_table, "Unable to grab memory map for ExitBootServices.\n");
+			break;
+		}
+		/*
+		 * Update the memory map with virtual addresses. The function
+		 * will also populate @runtime_map with copies of just the
+		 * EFI_MEMORY_RUNTIME entries so that we can pass it straight
+		 * into SetVirtualAddressMap()
+		 */
+		efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
+				&runtime_entry_count);
+
+		/* Now we are ready to exit_boot_services.*/
+		status = sys_table->boottime->exit_boot_services(handle,
+								 mmap_key);
+		/*
+		 * EFI_INVALID_PARAMETER means our memory map key does not
+		 * match the current map, likely because some event such as an
+		 * interrupt caused an allocation ourside of our control.  Thus
+		 * we need to get the updated map, and try again.  Our FDT
+		 * allocation from above remains valid, so no need to redo that.
+		 */
+	} while (status == EFI_INVALID_PARAMETER);
 
-	/* Now we are ready to exit_boot_services.*/
-	status = sys_table->boottime->exit_boot_services(handle, mmap_key);
 
 	if (status == EFI_SUCCESS) {
 		efi_set_virtual_address_map_t *svam;
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Mark Rutland]

Hi,

[Adding Ard and Leif]

On Thu, Jun 30, 2016 at 09:35:33AM -0600, Jeff Hugo wrote:
> From: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> There exists a race condition between when the efi stub grabs the memory
> map, and when ExitBootServices is called at which point the EFI can process
> an event which causes the memory map to be invalidated. 

For reference, do you have a particular example of such an event?

Do these events cause the memory map to grow?

> According to the UEFI spec, ExitBootServices will return
> EFI_INVALID_PARAMETER if this occurs, at which point the efi stub is
> expected to obtain the new memory map, and retry the call to
> ExitBootServices.
> 
> Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> 
> I'm not particularly happy with the current state of this fix, but I feel like
> this issue is somewhat unsolvable.  Currently I've based this solution upon
> arch/x86/boot/compressed/eboot.c however it has two primary issues I'd like
> feedback upon-
> 
> First issue-
> efi_get_memory_map() performs an allocation as it attempts to create a
> minimal sized buffer to hold the map.  Per my understanding of the UEFI spec,
> allocations are not permitted after a call to ExitBootServices:
> 
> "A UEFI OS loader should not make calls to any boot service function other than
> GetMemoryMap() after the first call to ExitBootServices()."

I see that appears on page 222 of the EFI 2.6 spec. To sve others from
digging, the relevant paragraph reads:

	A UEFI OS loader must ensure that it has the system’s current
	memory map at the time it calls ExitBootServices(). This is done
	by passing in the current memory map’s MapKey value as returned
	by EFI_BOOT_SERVICES.GetMemoryMap(). Care must be taken to
	ensure that the memory map does not change between these two
	calls. It is suggested that GetMemoryMap()be called immediately
	before calling ExitBootServices(). If MapKey value is incorrect,
	ExitBootServices() returns EFI_INVALID_PARAMETER and
	GetMemoryMap() with ExitBootServices() must be called again.
	Firmware implementation may choose to do a partial shutdown of
	the boot services during the first call to ExitBootServices(). A
	UEFI OS loader should not make calls to any boot service
	function other than GetMemoryMap() after the first call to
	ExitBootServices().

That "partial shutdown" also means that giving up after a failed
ExitBootServices() call is difficult. We can't log anything, and
whatever we return to can't call any boot services.

> However the only alternative I can think of it to allocate a sufficently large
> buffer so that it can be reused to hold the modified memory map.  There doesn't
> seem to be any limit to the new map, so any buffer space value I would choose
> would be arbitrary, and there would be some chance that it would be insufficent.
> efi_get_memory_map() would need to be modified to "return" the origional size
> of the allocated buffer as well, so I feel like this solution makes a mess of
> the code, reduces the value of the efi_get_memory_map() helper function, and for
> all that effort, we still can't fully address this race condition.
> 
> I guess the question is, where do we draw the line at "good enough" for
> addressing this issue?  Do we use efi_get_memory_map() since it appears to be
> cleaner and does not seem to cause a problem today, despite violating the spec?

We shouldn't be knowingly violating the UEFI spec.

At the very least, this should be raised with the USWG. This feels like
a specification bug, given that it's impossible (rather than simply
difficult) to solve the issue.

Ideally, we have the rules regarding a failed call to ExitBootServices()
tightened such that other boot services calls are valid. The current
wording appears to result in a number of unsolvable issues.

> Second issue-
> When do we give up if we cannot get a good memory map to use with
> ExitBootServices?  Currently there is an infinite loop in my patch.  I noticed
> arch/x86/boot/compressed/eboot.c only retrys after the first failure.  I think
> a single retry is insufficent, we could do better, but I'm aware that an
> infinite loop is generally a bad idea.  Any strong opinions on when to quit?
> 100 attempts?  Either way, it seems the system will require a hard reboot if
> the retry(s) ever end up failing.

I think this depends on what the problematic events are.

Thanks,
Mark.

> 
>  drivers/firmware/efi/libstub/fdt.c | 45 ++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index e58abfa..d3e70b0 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -250,16 +250,43 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  		}
>  	}
>  
> -	/*
> -	 * Update the memory map with virtual addresses. The function will also
> -	 * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
> -	 * entries so that we can pass it straight into SetVirtualAddressMap()
> -	 */
> -	efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
> -			&runtime_entry_count);
> +	do {
> +		/*
> +		 * We should not free and/or allocate after calling
> +		 * ExitBootServices, but this avoids the issue of guessing how
> +		 * large of a buffer we need to store an updated memory map,
> +		 * and its common for the implementation of ExitBootServices to
> +		 * first check if the map key is current, and error out early
> +		 * leaving all other services active.
> +		 */
> +		sys_table->boottime->free_pool(memory_map);
> +		status = efi_get_memory_map(sys_table, &memory_map, &map_size,
> +					    &desc_size, &desc_ver, &mmap_key);
> +		if (status != EFI_SUCCESS) {
> +			pr_efi_err(sys_table, "Unable to grab memory map for ExitBootServices.\n");
> +			break;
> +		}
> +		/*
> +		 * Update the memory map with virtual addresses. The function
> +		 * will also populate @runtime_map with copies of just the
> +		 * EFI_MEMORY_RUNTIME entries so that we can pass it straight
> +		 * into SetVirtualAddressMap()
> +		 */
> +		efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
> +				&runtime_entry_count);
> +
> +		/* Now we are ready to exit_boot_services.*/
> +		status = sys_table->boottime->exit_boot_services(handle,
> +								 mmap_key);
> +		/*
> +		 * EFI_INVALID_PARAMETER means our memory map key does not
> +		 * match the current map, likely because some event such as an
> +		 * interrupt caused an allocation ourside of our control.  Thus
> +		 * we need to get the updated map, and try again.  Our FDT
> +		 * allocation from above remains valid, so no need to redo that.
> +		 */
> +	} while (status == EFI_INVALID_PARAMETER);
>  
> -	/* Now we are ready to exit_boot_services.*/
> -	status = sys_table->boottime->exit_boot_services(handle, mmap_key);
>  
>  	if (status == EFI_SUCCESS) {
>  		efi_set_virtual_address_map_t *svam;
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Ard Biesheuvel]

On 30 June 2016 at 18:27, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi,
>
> [Adding Ard and Leif]
>
> On Thu, Jun 30, 2016 at 09:35:33AM -0600, Jeff Hugo wrote:
>> From: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>
>> There exists a race condition between when the efi stub grabs the memory
>> map, and when ExitBootServices is called at which point the EFI can process
>> an event which causes the memory map to be invalidated.
>
> For reference, do you have a particular example of such an event?
>
> Do these events cause the memory map to grow?
>

Events are typically allowed to allocate or free memory, unless they
have the EVT_SIGNAL_EXIT_BOOT_SERVICES attribute. Whether they cause
the memory map to grow is hard to predict, so one must assume yes.

>> According to the UEFI spec, ExitBootServices will return
>> EFI_INVALID_PARAMETER if this occurs, at which point the efi stub is
>> expected to obtain the new memory map, and retry the call to
>> ExitBootServices.
>>
>> Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>
>> I'm not particularly happy with the current state of this fix, but I feel like
>> this issue is somewhat unsolvable.  Currently I've based this solution upon
>> arch/x86/boot/compressed/eboot.c however it has two primary issues I'd like
>> feedback upon-
>>
>> First issue-
>> efi_get_memory_map() performs an allocation as it attempts to create a
>> minimal sized buffer to hold the map.  Per my understanding of the UEFI spec,
>> allocations are not permitted after a call to ExitBootServices:
>>
>> "A UEFI OS loader should not make calls to any boot service function other than
>> GetMemoryMap() after the first call to ExitBootServices()."
>
> I see that appears on page 222 of the EFI 2.6 spec. To sve others from
> digging, the relevant paragraph reads:
>
>         A UEFI OS loader must ensure that it has the system’s current
>         memory map at the time it calls ExitBootServices(). This is done
>         by passing in the current memory map’s MapKey value as returned
>         by EFI_BOOT_SERVICES.GetMemoryMap(). Care must be taken to
>         ensure that the memory map does not change between these two
>         calls. It is suggested that GetMemoryMap()be called immediately
>         before calling ExitBootServices(). If MapKey value is incorrect,
>         ExitBootServices() returns EFI_INVALID_PARAMETER and
>         GetMemoryMap() with ExitBootServices() must be called again.
>         Firmware implementation may choose to do a partial shutdown of
>         the boot services during the first call to ExitBootServices(). A
>         UEFI OS loader should not make calls to any boot service
>         function other than GetMemoryMap() after the first call to
>         ExitBootServices().
>
> That "partial shutdown" also means that giving up after a failed
> ExitBootServices() call is difficult. We can't log anything, and
> whatever we return to can't call any boot services.
>

This is the unfortunate part: we lost our console so there is nothing
we can do except hang, or proceed without a memory map. Since we have
already allocated space for the static kernel image in this case, it
may be better to proceed so we can at least die loudly on earlycon
enabled configurations.

>> However the only alternative I can think of it to allocate a sufficently large
>> buffer so that it can be reused to hold the modified memory map.  There doesn't
>> seem to be any limit to the new map, so any buffer space value I would choose
>> would be arbitrary, and there would be some chance that it would be insufficent.
>> efi_get_memory_map() would need to be modified to "return" the origional size
>> of the allocated buffer as well, so I feel like this solution makes a mess of
>> the code, reduces the value of the efi_get_memory_map() helper function, and for
>> all that effort, we still can't fully address this race condition.
>>
>> I guess the question is, where do we draw the line at "good enough" for
>> addressing this issue?  Do we use efi_get_memory_map() since it appears to be
>> cleaner and does not seem to cause a problem today, despite violating the spec?
>
> We shouldn't be knowingly violating the UEFI spec.
>
> At the very least, this should be raised with the USWG. This feels like
> a specification bug, given that it's impossible (rather than simply
> difficult) to solve the issue.
>

efi_get_memory_map() is the linux wrapper around the GetMemoryMap()
boot service, and the latter does not perform any allocations. The
spec also clearly states that GetMemoryMap() can be called after EBS()
has failed.

> Ideally, we have the rules regarding a failed call to ExitBootServices()
> tightened such that other boot services calls are valid. The current
> wording appears to result in a number of unsolvable issues.
>

The only unsolvable issue is that we may find that we did not allocate
sufficient slack to cover the updated memory map. Typically, a
periodic event that happens to allocate and/or free some memory in its
handler may result in one or two entries to be added, but it is
bounded in practice even if the spec does not spell it out explicitly.

So allocating a couple of entries' worth of slack should be sufficient
in virtually all cases.

>> Second issue-
>> When do we give up if we cannot get a good memory map to use with
>> ExitBootServices?  Currently there is an infinite loop in my patch.  I noticed
>> arch/x86/boot/compressed/eboot.c only retrys after the first failure.  I think
>> a single retry is insufficent, we could do better, but I'm aware that an
>> infinite loop is generally a bad idea.  Any strong opinions on when to quit?
>> 100 attempts?  Either way, it seems the system will require a hard reboot if
>> the retry(s) ever end up failing.
>
> I think this depends on what the problematic events are.
>

The wording of the spec suggests that two attempts at the most covers
all cases, and the EDK2 implementation confirms that: the first thing
it does is disarm the timer, and since all asynchronous processing in
UEFI is event based (no interrupts except for the timer or for debug),
this guarantees that the race condition that hit us the first time
does not exist anymore the second time around.

I know this is all a bit hand wavy, but I never experienced the issue
in practice (to my knowledge) and I don't think it makes a huge lot of
sense to complicate this code too much only to cater for theoretical
spec violations.

-- 
Ard.

>>
>>  drivers/firmware/efi/libstub/fdt.c | 45 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 36 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>> index e58abfa..d3e70b0 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -250,16 +250,43 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>               }
>>       }
>>
>> -     /*
>> -      * Update the memory map with virtual addresses. The function will also
>> -      * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
>> -      * entries so that we can pass it straight into SetVirtualAddressMap()
>> -      */
>> -     efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
>> -                     &runtime_entry_count);
>> +     do {
>> +             /*
>> +              * We should not free and/or allocate after calling
>> +              * ExitBootServices, but this avoids the issue of guessing how
>> +              * large of a buffer we need to store an updated memory map,
>> +              * and its common for the implementation of ExitBootServices to
>> +              * first check if the map key is current, and error out early
>> +              * leaving all other services active.
>> +              */
>> +             sys_table->boottime->free_pool(memory_map);
>> +             status = efi_get_memory_map(sys_table, &memory_map, &map_size,
>> +                                         &desc_size, &desc_ver, &mmap_key);
>> +             if (status != EFI_SUCCESS) {
>> +                     pr_efi_err(sys_table, "Unable to grab memory map for ExitBootServices.\n");
>> +                     break;
>> +             }
>> +             /*
>> +              * Update the memory map with virtual addresses. The function
>> +              * will also populate @runtime_map with copies of just the
>> +              * EFI_MEMORY_RUNTIME entries so that we can pass it straight
>> +              * into SetVirtualAddressMap()
>> +              */
>> +             efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
>> +                             &runtime_entry_count);
>> +
>> +             /* Now we are ready to exit_boot_services.*/
>> +             status = sys_table->boottime->exit_boot_services(handle,
>> +                                                              mmap_key);
>> +             /*
>> +              * EFI_INVALID_PARAMETER means our memory map key does not
>> +              * match the current map, likely because some event such as an
>> +              * interrupt caused an allocation ourside of our control.  Thus
>> +              * we need to get the updated map, and try again.  Our FDT
>> +              * allocation from above remains valid, so no need to redo that.
>> +              */
>> +     } while (status == EFI_INVALID_PARAMETER);
>>
>> -     /* Now we are ready to exit_boot_services.*/
>> -     status = sys_table->boottime->exit_boot_services(handle, mmap_key);
>>
>>       if (status == EFI_SUCCESS) {
>>               efi_set_virtual_address_map_t *svam;
>> --
>> Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Jeffrey Hugo]

On 6/30/2016 10:47 AM, Ard Biesheuvel wrote:
> On 30 June 2016 at 18:27, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> Hi,
>>
>> [Adding Ard and Leif]
>>
>> On Thu, Jun 30, 2016 at 09:35:33AM -0600, Jeff Hugo wrote:
>>> From: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>>
>>> There exists a race condition between when the efi stub grabs the memory
>>> map, and when ExitBootServices is called at which point the EFI can process
>>> an event which causes the memory map to be invalidated.
>>
>> For reference, do you have a particular example of such an event?
>>
>> Do these events cause the memory map to grow?
>>
>
> Events are typically allowed to allocate or free memory, unless they
> have the EVT_SIGNAL_EXIT_BOOT_SERVICES attribute. Whether they cause
> the memory map to grow is hard to predict, so one must assume yes.
>
>>> According to the UEFI spec, ExitBootServices will return
>>> EFI_INVALID_PARAMETER if this occurs, at which point the efi stub is
>>> expected to obtain the new memory map, and retry the call to
>>> ExitBootServices.
>>>
>>> Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>> ---
>>>
>>> I'm not particularly happy with the current state of this fix, but I feel like
>>> this issue is somewhat unsolvable.  Currently I've based this solution upon
>>> arch/x86/boot/compressed/eboot.c however it has two primary issues I'd like
>>> feedback upon-
>>>
>>> First issue-
>>> efi_get_memory_map() performs an allocation as it attempts to create a
>>> minimal sized buffer to hold the map.  Per my understanding of the UEFI spec,
>>> allocations are not permitted after a call to ExitBootServices:
>>>
>>> "A UEFI OS loader should not make calls to any boot service function other than
>>> GetMemoryMap() after the first call to ExitBootServices()."
>>
>> I see that appears on page 222 of the EFI 2.6 spec. To sve others from
>> digging, the relevant paragraph reads:
>>
>>         A UEFI OS loader must ensure that it has the system’s current
>>         memory map at the time it calls ExitBootServices(). This is done
>>         by passing in the current memory map’s MapKey value as returned
>>         by EFI_BOOT_SERVICES.GetMemoryMap(). Care must be taken to
>>         ensure that the memory map does not change between these two
>>         calls. It is suggested that GetMemoryMap()be called immediately
>>         before calling ExitBootServices(). If MapKey value is incorrect,
>>         ExitBootServices() returns EFI_INVALID_PARAMETER and
>>         GetMemoryMap() with ExitBootServices() must be called again.
>>         Firmware implementation may choose to do a partial shutdown of
>>         the boot services during the first call to ExitBootServices(). A
>>         UEFI OS loader should not make calls to any boot service
>>         function other than GetMemoryMap() after the first call to
>>         ExitBootServices().
>>
>> That "partial shutdown" also means that giving up after a failed
>> ExitBootServices() call is difficult. We can't log anything, and
>> whatever we return to can't call any boot services.
>>
>
> This is the unfortunate part: we lost our console so there is nothing
> we can do except hang, or proceed without a memory map. Since we have
> already allocated space for the static kernel image in this case, it
> may be better to proceed so we can at least die loudly on earlycon
> enabled configurations.
>
>>> However the only alternative I can think of it to allocate a sufficently large
>>> buffer so that it can be reused to hold the modified memory map.  There doesn't
>>> seem to be any limit to the new map, so any buffer space value I would choose
>>> would be arbitrary, and there would be some chance that it would be insufficent.
>>> efi_get_memory_map() would need to be modified to "return" the origional size
>>> of the allocated buffer as well, so I feel like this solution makes a mess of
>>> the code, reduces the value of the efi_get_memory_map() helper function, and for
>>> all that effort, we still can't fully address this race condition.
>>>
>>> I guess the question is, where do we draw the line at "good enough" for
>>> addressing this issue?  Do we use efi_get_memory_map() since it appears to be
>>> cleaner and does not seem to cause a problem today, despite violating the spec?
>>
>> We shouldn't be knowingly violating the UEFI spec.
>>
>> At the very least, this should be raised with the USWG. This feels like
>> a specification bug, given that it's impossible (rather than simply
>> difficult) to solve the issue.
>>
>
> efi_get_memory_map() is the linux wrapper around the GetMemoryMap()
> boot service, and the latter does not perform any allocations. The
> spec also clearly states that GetMemoryMap() can be called after EBS()
> has failed.
>
>> Ideally, we have the rules regarding a failed call to ExitBootServices()
>> tightened such that other boot services calls are valid. The current
>> wording appears to result in a number of unsolvable issues.
>>
>
> The only unsolvable issue is that we may find that we did not allocate
> sufficient slack to cover the updated memory map. Typically, a
> periodic event that happens to allocate and/or free some memory in its
> handler may result in one or two entries to be added, but it is
> bounded in practice even if the spec does not spell it out explicitly.
>
> So allocating a couple of entries' worth of slack should be sufficient
> in virtually all cases.

True.  I've talked with the folks implementing UEFI on my test platform, 
and they believe a buffer of 8 entries would be more than sufficient for 
all practical purposes.

>
>>> Second issue-
>>> When do we give up if we cannot get a good memory map to use with
>>> ExitBootServices?  Currently there is an infinite loop in my patch.  I noticed
>>> arch/x86/boot/compressed/eboot.c only retrys after the first failure.  I think
>>> a single retry is insufficent, we could do better, but I'm aware that an
>>> infinite loop is generally a bad idea.  Any strong opinions on when to quit?
>>> 100 attempts?  Either way, it seems the system will require a hard reboot if
>>> the retry(s) ever end up failing.
>>
>> I think this depends on what the problematic events are.
>>
>
> The wording of the spec suggests that two attempts at the most covers
> all cases, and the EDK2 implementation confirms that: the first thing
> it does is disarm the timer, and since all asynchronous processing in
> UEFI is event based (no interrupts except for the timer or for debug),
> this guarantees that the race condition that hit us the first time
> does not exist anymore the second time around.
>
> I know this is all a bit hand wavy, but I never experienced the issue
> in practice (to my knowledge) and I don't think it makes a huge lot of
> sense to complicate this code too much only to cater for theoretical
> spec violations.
>

On the platform I'm testing, UEFI follows the EDK2 implementation, so 
I've been only able to trigger the failure initially by putting in a 
"sleep" between grabbing the map/ExitBootServices() and plugging and/or 
unplugging in a usb flash stick into the device.  The issue does not 
reoccur after ExitBootServices() is called and fails.

I do have a few reports of the issue occurring "in the wild", so I am 
looking to address it.  Based on the data I have, its rare, less than 1%.

I understand your position.  On unrelated issues/projects I have been 
burned where something gets fixed to address an issue with today's 
implementation, and then it breaks again because that implementation 
changed in a way that was allowed by the relevant spec.  Based on those 
experiences, I'm wary to just say "welp it works today", but this 
segment of code does look short and to the point, so I'm kind of leaning 
towards your position, I'd prefer not to complicate it without a strong 
reason.

I admit, this is not my domain of expertise, I'm just trying to solve an 
issue that was reported to me.  What would be your preference for a 
solution?  A single retry if ExitBootServices() fails using 
efi_get_memory_map() - basically an exact copy of how this appears to be 
solved in arch/x86/boot/compressed/eboot.c?

-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Ard Biesheuvel]

On 30 June 2016 at 19:56, Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> On 6/30/2016 10:47 AM, Ard Biesheuvel wrote:
>>
>> On 30 June 2016 at 18:27, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>>>
>>> Hi,
>>>
>>> [Adding Ard and Leif]
>>>
>>> On Thu, Jun 30, 2016 at 09:35:33AM -0600, Jeff Hugo wrote:
>>>>
>>>> From: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>>>
>>>> There exists a race condition between when the efi stub grabs the memory
>>>> map, and when ExitBootServices is called at which point the EFI can
>>>> process
>>>> an event which causes the memory map to be invalidated.
>>>
>>>
>>> For reference, do you have a particular example of such an event?
>>>
>>> Do these events cause the memory map to grow?
>>>
>>
>> Events are typically allowed to allocate or free memory, unless they
>> have the EVT_SIGNAL_EXIT_BOOT_SERVICES attribute. Whether they cause
>> the memory map to grow is hard to predict, so one must assume yes.
>>
>>>> According to the UEFI spec, ExitBootServices will return
>>>> EFI_INVALID_PARAMETER if this occurs, at which point the efi stub is
>>>> expected to obtain the new memory map, and retry the call to
>>>> ExitBootServices.
>>>>
>>>> Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>>> ---
>>>>
>>>> I'm not particularly happy with the current state of this fix, but I
>>>> feel like
>>>> this issue is somewhat unsolvable.  Currently I've based this solution
>>>> upon
>>>> arch/x86/boot/compressed/eboot.c however it has two primary issues I'd
>>>> like
>>>> feedback upon-
>>>>
>>>> First issue-
>>>> efi_get_memory_map() performs an allocation as it attempts to create a
>>>> minimal sized buffer to hold the map.  Per my understanding of the UEFI
>>>> spec,
>>>> allocations are not permitted after a call to ExitBootServices:
>>>>
>>>> "A UEFI OS loader should not make calls to any boot service function
>>>> other than
>>>> GetMemoryMap() after the first call to ExitBootServices()."
>>>
>>>
>>> I see that appears on page 222 of the EFI 2.6 spec. To sve others from
>>> digging, the relevant paragraph reads:
>>>
>>>         A UEFI OS loader must ensure that it has the system’s current
>>>         memory map at the time it calls ExitBootServices(). This is done
>>>         by passing in the current memory map’s MapKey value as returned
>>>         by EFI_BOOT_SERVICES.GetMemoryMap(). Care must be taken to
>>>         ensure that the memory map does not change between these two
>>>         calls. It is suggested that GetMemoryMap()be called immediately
>>>         before calling ExitBootServices(). If MapKey value is incorrect,
>>>         ExitBootServices() returns EFI_INVALID_PARAMETER and
>>>         GetMemoryMap() with ExitBootServices() must be called again.
>>>         Firmware implementation may choose to do a partial shutdown of
>>>         the boot services during the first call to ExitBootServices(). A
>>>         UEFI OS loader should not make calls to any boot service
>>>         function other than GetMemoryMap() after the first call to
>>>         ExitBootServices().
>>>
>>> That "partial shutdown" also means that giving up after a failed
>>> ExitBootServices() call is difficult. We can't log anything, and
>>> whatever we return to can't call any boot services.
>>>
>>
>> This is the unfortunate part: we lost our console so there is nothing
>> we can do except hang, or proceed without a memory map. Since we have
>> already allocated space for the static kernel image in this case, it
>> may be better to proceed so we can at least die loudly on earlycon
>> enabled configurations.
>>
>>>> However the only alternative I can think of it to allocate a sufficently
>>>> large
>>>> buffer so that it can be reused to hold the modified memory map.  There
>>>> doesn't
>>>> seem to be any limit to the new map, so any buffer space value I would
>>>> choose
>>>> would be arbitrary, and there would be some chance that it would be
>>>> insufficent.
>>>> efi_get_memory_map() would need to be modified to "return" the origional
>>>> size
>>>> of the allocated buffer as well, so I feel like this solution makes a
>>>> mess of
>>>> the code, reduces the value of the efi_get_memory_map() helper function,
>>>> and for
>>>> all that effort, we still can't fully address this race condition.
>>>>
>>>> I guess the question is, where do we draw the line at "good enough" for
>>>> addressing this issue?  Do we use efi_get_memory_map() since it appears
>>>> to be
>>>> cleaner and does not seem to cause a problem today, despite violating
>>>> the spec?
>>>
>>>
>>> We shouldn't be knowingly violating the UEFI spec.
>>>
>>> At the very least, this should be raised with the USWG. This feels like
>>> a specification bug, given that it's impossible (rather than simply
>>> difficult) to solve the issue.
>>>
>>
>> efi_get_memory_map() is the linux wrapper around the GetMemoryMap()
>> boot service, and the latter does not perform any allocations. The
>> spec also clearly states that GetMemoryMap() can be called after EBS()
>> has failed.
>>
>>> Ideally, we have the rules regarding a failed call to ExitBootServices()
>>> tightened such that other boot services calls are valid. The current
>>> wording appears to result in a number of unsolvable issues.
>>>
>>
>> The only unsolvable issue is that we may find that we did not allocate
>> sufficient slack to cover the updated memory map. Typically, a
>> periodic event that happens to allocate and/or free some memory in its
>> handler may result in one or two entries to be added, but it is
>> bounded in practice even if the spec does not spell it out explicitly.
>>
>> So allocating a couple of entries' worth of slack should be sufficient
>> in virtually all cases.
>
>
> True.  I've talked with the folks implementing UEFI on my test platform, and
> they believe a buffer of 8 entries would be more than sufficient for all
> practical purposes.
>

OK, so we're on the same page here

>>
>>>> Second issue-
>>>> When do we give up if we cannot get a good memory map to use with
>>>> ExitBootServices?  Currently there is an infinite loop in my patch.  I
>>>> noticed
>>>> arch/x86/boot/compressed/eboot.c only retrys after the first failure.  I
>>>> think
>>>> a single retry is insufficent, we could do better, but I'm aware that an
>>>> infinite loop is generally a bad idea.  Any strong opinions on when to
>>>> quit?
>>>> 100 attempts?  Either way, it seems the system will require a hard
>>>> reboot if
>>>> the retry(s) ever end up failing.
>>>
>>>
>>> I think this depends on what the problematic events are.
>>>
>>
>> The wording of the spec suggests that two attempts at the most covers
>> all cases, and the EDK2 implementation confirms that: the first thing
>> it does is disarm the timer, and since all asynchronous processing in
>> UEFI is event based (no interrupts except for the timer or for debug),
>> this guarantees that the race condition that hit us the first time
>> does not exist anymore the second time around.
>>
>> I know this is all a bit hand wavy, but I never experienced the issue
>> in practice (to my knowledge) and I don't think it makes a huge lot of
>> sense to complicate this code too much only to cater for theoretical
>> spec violations.
>>
>
> On the platform I'm testing, UEFI follows the EDK2 implementation, so I've
> been only able to trigger the failure initially by putting in a "sleep"
> between grabbing the map/ExitBootServices() and plugging and/or unplugging
> in a usb flash stick into the device.  The issue does not reoccur after
> ExitBootServices() is called and fails.
>
> I do have a few reports of the issue occurring "in the wild", so I am
> looking to address it.  Based on the data I have, its rare, less than 1%.
>
> I understand your position.  On unrelated issues/projects I have been burned
> where something gets fixed to address an issue with today's implementation,
> and then it breaks again because that implementation changed in a way that
> was allowed by the relevant spec.  Based on those experiences, I'm wary to
> just say "welp it works today", but this segment of code does look short and
> to the point, so I'm kind of leaning towards your position, I'd prefer not
> to complicate it without a strong reason.
>
> I admit, this is not my domain of expertise, I'm just trying to solve an
> issue that was reported to me.  What would be your preference for a
> solution?  A single retry if ExitBootServices() fails using
> efi_get_memory_map() - basically an exact copy of how this appears to be
> solved in arch/x86/boot/compressed/eboot.c?
>

No, I think x86's implementation is incorrect. efi_get_memory_map()
should allocate some slack (i.e., the 8 entries you mentioned), and if
the first call to ExitBootServices() fails, we should reuse the memory
map buffer, and call GetMemoryMap() directly to repopulate it. Then,
we call ExitBootServices() once more, or give up if either call fails.

This way, we are 100% compliant with the wording of the spec, and err
on the side of caution.

-- 
Ard.

Re: [RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Jeffrey Hugo]

On 6/30/2016 12:31 PM, Ard Biesheuvel wrote:
>
> No, I think x86's implementation is incorrect. efi_get_memory_map()
> should allocate some slack (i.e., the 8 entries you mentioned), and if
> the first call to ExitBootServices() fails, we should reuse the memory
> map buffer, and call GetMemoryMap() directly to repopulate it. Then,
> we call ExitBootServices() once more, or give up if either call fails.
>
> This way, we are 100% compliant with the wording of the spec, and err
> on the side of caution.
>

Ok.  Let me take some time to think upon this approach and develop a 
working prototype.

Thanks for providing your guidance in a very expedient manner.

-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Matt Fleming]

On Thu, 30 Jun, at 03:05:12PM, Jeffrey Hugo wrote:
> On 6/30/2016 12:31 PM, Ard Biesheuvel wrote:
> >
> >No, I think x86's implementation is incorrect. efi_get_memory_map()
> >should allocate some slack (i.e., the 8 entries you mentioned), and if
> >the first call to ExitBootServices() fails, we should reuse the memory
> >map buffer, and call GetMemoryMap() directly to repopulate it. Then,
> >we call ExitBootServices() once more, or give up if either call fails.
> >
> >This way, we are 100% compliant with the wording of the spec, and err
> >on the side of caution.
> >
> 
> Ok.  Let me take some time to think upon this approach and develop a working
> prototype.
 
If possible I'd much prefer to see this fixed only once in the
shared part of efi/libstub, so that the code in arch/x86 can be
deleted in favour of your more robust patch.

FYI, commit d3768d885c6c ("x86, efi: retry ExitBootServices() on
failure") illustrates that we have hit this bug in the wild on x86.

Re: [RFC PATCH] efi/libstub: Retry ExitBootServices if map key is invalid

[Mark Rutland]

On Thu, Jun 30, 2016 at 06:47:02PM +0200, Ard Biesheuvel wrote:
> On 30 June 2016 at 18:27, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi,
> >
> > [Adding Ard and Leif]
> >
> > On Thu, Jun 30, 2016 at 09:35:33AM -0600, Jeff Hugo wrote:
> >> From: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> >>
> >> There exists a race condition between when the efi stub grabs the memory
> >> map, and when ExitBootServices is called at which point the EFI can process
> >> an event which causes the memory map to be invalidated.
> >
> > For reference, do you have a particular example of such an event?
> >
> > Do these events cause the memory map to grow?
> >
> 
> Events are typically allowed to allocate or free memory, unless they
> have the EVT_SIGNAL_EXIT_BOOT_SERVICES attribute. Whether they cause
> the memory map to grow is hard to predict, so one must assume yes.

Ok.
 
> > That "partial shutdown" also means that giving up after a failed
> > ExitBootServices() call is difficult. We can't log anything, and
> > whatever we return to can't call any boot services.
> 
> This is the unfortunate part: we lost our console so there is nothing
> we can do except hang, or proceed without a memory map. Since we have
> already allocated space for the static kernel image in this case, it
> may be better to proceed so we can at least die loudly on earlycon
> enabled configurations.

Hmm... That might be best, assuming we can somehow signal to the kernel
that something hass gone very wrong.

> >> However the only alternative I can think of it to allocate a sufficently large
> >> buffer so that it can be reused to hold the modified memory map.  There doesn't
> >> seem to be any limit to the new map, so any buffer space value I would choose
> >> would be arbitrary, and there would be some chance that it would be insufficent.
> >> efi_get_memory_map() would need to be modified to "return" the origional size
> >> of the allocated buffer as well, so I feel like this solution makes a mess of
> >> the code, reduces the value of the efi_get_memory_map() helper function, and for
> >> all that effort, we still can't fully address this race condition.
> >>
> >> I guess the question is, where do we draw the line at "good enough" for
> >> addressing this issue?  Do we use efi_get_memory_map() since it appears to be
> >> cleaner and does not seem to cause a problem today, despite violating the spec?
> >
> > We shouldn't be knowingly violating the UEFI spec.
> >
> > At the very least, this should be raised with the USWG. This feels like
> > a specification bug, given that it's impossible (rather than simply
> > difficult) to solve the issue.
> 
> efi_get_memory_map() is the linux wrapper around the GetMemoryMap()
> boot service, and the latter does not perform any allocations. The
> spec also clearly states that GetMemoryMap() can be called after EBS()
> has failed.

Sure, I understood that.

I find it surprising that the spec rules out the use of the memory
allocation services that efi_get_memory_map() uses under the hood, given
that I imagine those don't require asynchronous processing, and having
those would cater for more extreme cases.

> > Ideally, we have the rules regarding a failed call to ExitBootServices()
> > tightened such that other boot services calls are valid. The current
> > wording appears to result in a number of unsolvable issues.
> 
> The only unsolvable issue is that we may find that we did not allocate
> sufficient slack to cover the updated memory map. Typically, a
> periodic event that happens to allocate and/or free some memory in its
> handler may result in one or two entries to be added, but it is
> bounded in practice even if the spec does not spell it out explicitly.
> 
> So allocating a couple of entries' worth of slack should be sufficient
> in virtually all cases.

I agree that this is likely to work in practice, given some arbitrary
amount of slack.

I still think it's worth poking the USWG about this, as they may care
about fixing this in the more general case spec-side. Even if we can't
change things, it seems worth a note that the size of the memory map may
grow.

> >> Second issue-
> >> When do we give up if we cannot get a good memory map to use with
> >> ExitBootServices?  Currently there is an infinite loop in my patch.  I noticed
> >> arch/x86/boot/compressed/eboot.c only retrys after the first failure.  I think
> >> a single retry is insufficent, we could do better, but I'm aware that an
> >> infinite loop is generally a bad idea.  Any strong opinions on when to quit?
> >> 100 attempts?  Either way, it seems the system will require a hard reboot if
> >> the retry(s) ever end up failing.
> >
> > I think this depends on what the problematic events are.
> 
> The wording of the spec suggests that two attempts at the most covers
> all cases, and the EDK2 implementation confirms that: the first thing
> it does is disarm the timer, and since all asynchronous processing in
> UEFI is event based (no interrupts except for the timer or for debug),
> this guarantees that the race condition that hit us the first time
> does not exist anymore the second time around.
> 
> I know this is all a bit hand wavy, but I never experienced the issue
> in practice (to my knowledge) and I don't think it makes a huge lot of
> sense to complicate this code too much only to cater for theoretical
> spec violations.

Understood.

Thanks,
Mark.