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

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

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.