On 16 March 2017 at 23:07, Roy Franz <rfranz@xxxxxxxxxx> wrote: > On Thu, Mar 16, 2017 at 3:02 PM, Ard Biesheuvel > <ard.biesheuvel@xxxxxxxxxx> wrote: >> On 16 March 2017 at 21:56, Roy Franz <rfranz@xxxxxxxxxx> wrote: >>> On Thu, Mar 16, 2017 at 4:26 AM, Leif Lindholm <leif.lindholm@xxxxxxxxxx> wrote: >>>> On Thu, Mar 16, 2017 at 10:56:16AM +0000, Ard Biesheuvel wrote: >>>>> The arm32 kernel decompresses itself to the base of DRAM unconditionally, >>>>> and so it is the EFI stub's job to ensure that the region is available. >>>>> >>>>> Currently, we do this by creating an allocation there, and giving up if >>>>> that fails. However, any boot services regions occupying this area are >>>>> not an issue, given that the decompressor executes strictly after the >>>>> stub calls ExitBootServices(). >>>>> >>>>> So let's try a bit harder to proceed if the initial allocation fails, >>>>> and check whether any memory map entries occupying the region may be >>>>> considered safe. >>>>> >>>>> Reported-by: Eugene Cohen <eugene@xxxxxx> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> >>>>> --- >>>>> >>>>> NOTE: This patch appears to have uncovered a bug in DxeCore's AllocatePages >>>>> routine. If the first allocate_pages(EFI_ALLOCATE_ADDRESS) call fails, we may >>>>> still end up with a memory map that reflects a kind of limbo state where the >>>>> intended allocation is carved out and partially converted. >>>>> >>>>> For example, starting from >>>>> >>>>> 0x000040000000-0x00004007ffff [ConventionalMemory ] >>>>> 0x000040080000-0x00004009ffff [Boot Data ] >>>>> 0x0000400a0000-0x000047ffffff [ConventionalMemory ] >>>>> >>>>> the failed allocation of 32 MB of LoaderData @ 0x4000_0000 will result in >>>>> >>>>> 0x000040000000-0x00004007ffff [Loader Data ] >>>>> 0x000040080000-0x00004009ffff [Boot Data ] >>>>> 0x0000400a0000-0x000047ffffff [ConventionalMemory ] >>>>> >>>>> after which scanning the region for LoaderData regions (which we should reject >>>>> given that they could be freed and replaced with, e.g., runtime services data >>>>> regions) will always fail. >>>>> >>>>> For this reason, the allocate_pages(EFI_ALLOCATE_ADDRESS) has been modified to >>>>> use EfiBootServicesData instead. In the mean time, I will report this to the >>>>> EDK2 development mailing list. >>>> >>>> That feels a little bit eeew, but I can't see it breaking anything. >>>> >>> (Sorry for HTML the first time...) >>> >>> Would it be reasonable to do a trial allocation with >>> EFI_BOOT_SERVICES_DATA first, and if that passes then free >>> it and do the allocation with EFI_LOADER_DATA? This keeps the final >>> allocation of the desired type, >>> but since this allocation is short lived I don't think it matters that much. >>> If left as is, I think it deserves a comment in the source so this >>> non-standard allocation is identified (and not copied). >>> >>> Also, if the fallback case fails, we may leave a bunch of memory allocated, >>> which could interfere with something >>> run after a failed kernel boot. I'm guessing in practice there won't be >>> anything run that needs a lot of memory, >>> but we're not being a good citizen in this regard. >>> >> >> The thing is that we are not actually allocating anything there, we >> only allocate it to ensure that nothing else uses the region. This >> means the second time around (e.g., if the stub exits to the uefi >> shell for some reason, and is invoked again), we will not reserve even >> more memory, but simply notice that the region we want to claim for >> the decompressed kernels has already been claimed. > > We're calling allocate_pages(), so we are allocating memory, right? > I'm thinking more of some other application that would allocate memory, > such as a diagnostic, etc - this memory won't be available to that > application if > it is run after a failed boot that allocates memory during the fallback loop. > I don't think the stub will re-use any previously allocated memory > on a second attempt - it allocates as EFI_LOADER_DATA, and in the > loop that checks memory EFI_LOADER_DATA is treated as unusable. > No, that is not what it does. The 32 MB allocation for the uncompressed kernel always resides at the base of DRAM, regardles of how many boot attempts you have made. The reason EFI_LOADER_DATA is treated as unusable is that such allocations may be freed before ExitBootServices(), which means the space they occupy may be given the other allocations, which is a problem if those are of a runtime type. So if the stub behaves as we think it does, all loader data allocations will be freed again if the stub exists without booting the kernel. The 32 MB block will not be released, but additional attempts to boot will simply notice that the 32 MB block is already covered by a memory allocation of a type that does not conflict with the uncompressed kernel, and not allocate any additional memory for it. > Neither of these things is likely a big deal - not much that would be run after > a failed Linux boot will need this memory, and unless action could be taken to > free the memory that caused the full-size allocation to fail addition > boot attempts > wouldn't be successful anyway. > True. -- 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
