| Message ID | 1489661776-12946-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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@hp.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > > 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. > drivers/firmware/efi/libstub/arm32-stub.c | 137 +++++++++++++++++--- > 1 file changed, 117 insertions(+), 20 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c > index e1f0b28e1dcb..4e1b6478986e 100644 > --- a/drivers/firmware/efi/libstub/arm32-stub.c > +++ b/drivers/firmware/efi/libstub/arm32-stub.c > @@ -63,6 +63,121 @@ void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) > efi_call_early(free_pool, si); > } > > +static efi_status_t reserve_kernel_base(efi_system_table_t *sys_table_arg, > + unsigned long dram_base, > + unsigned long *reserve_addr, > + unsigned long *reserve_size) > +{ > + efi_physical_addr_t alloc_addr; > + efi_memory_desc_t *memory_map; > + unsigned long nr_pages, map_size, desc_size, buff_size; > + efi_status_t status; > + unsigned long l; > + > + struct efi_boot_memmap map = { > + .map = &memory_map, > + .map_size = &map_size, > + .desc_size = &desc_size, > + .desc_ver = NULL, > + .key_ptr = NULL, > + .buff_size = &buff_size, > + }; > + > + /* > + * Reserve memory for the uncompressed kernel image. This is > + * all that prevents any future allocations from conflicting > + * with the kernel. Since we can't tell from the compressed > + * image how much DRAM the kernel actually uses (due to BSS > + * size uncertainty) we allocate the maximum possible size. > + * Do this very early, as prints can cause memory allocations > + * that may conflict with this. > + */ > + alloc_addr = dram_base; > + *reserve_size = MAX_UNCOMP_KERNEL_SIZE; > + nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; > + status = sys_table_arg->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, > + EFI_BOOT_SERVICES_DATA, > + nr_pages, &alloc_addr); > + if (status == EFI_SUCCESS) { > + *reserve_addr = alloc_addr; > + return EFI_SUCCESS; > + } > + > + /* > + * If the allocation above failed, we may still be able to proceed: > + * if the only allocations in the region are of types that will be > + * released to the OS after ExitBootServices(), the decompressor can > + * safely overwrite them. > + */ > + status = efi_get_memory_map(sys_table_arg, &map); > + if (status != EFI_SUCCESS) { > + pr_efi_err(sys_table_arg, > + "reserve_kernel_base(): Unable to retrieve memory map.\n"); > + return status; > + } > + > + for (l = 0; l < map_size; l += desc_size) { > + efi_memory_desc_t *desc; > + u64 start, end; > + > + desc = (void *)memory_map + l; > + start = desc->phys_addr; > + end = start + desc->num_pages * EFI_PAGE_SIZE; > + > + /* does this entry cover the region? */ Nitpick: the logic tests the opposite of what the comment describes. /* Skip if entry does not intersect with region */ ? Anyway, that's minor. Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > + if (start >= dram_base + MAX_UNCOMP_KERNEL_SIZE || > + end <= dram_base) > + continue; > + > + /* ignore types that are released to the OS anyway */ > + switch (desc->type) { > + case EFI_BOOT_SERVICES_CODE: > + case EFI_BOOT_SERVICES_DATA: > + /* these are safe -- ignore */ > + continue; > + > + case EFI_CONVENTIONAL_MEMORY: > + /* > + * Reserve the intersection between this entry and the > + * region. > + */ > + start = max(start, (u64)dram_base); > + end = min(end, (u64)dram_base + MAX_UNCOMP_KERNEL_SIZE); > + > + status = efi_call_early(allocate_pages, > + EFI_ALLOCATE_ADDRESS, > + EFI_LOADER_DATA, > + (end - start) / EFI_PAGE_SIZE, > + &start); > + if (status != EFI_SUCCESS) { > + pr_efi_err(sys_table_arg, > + "reserve_kernel_base(): alloc failed.\n"); > + goto out; > + } > + break; > + > + case EFI_LOADER_CODE: > + case EFI_LOADER_DATA: > + /* > + * These regions may be released and reallocated for > + * another purpose (including EFI_RUNTIME_SERVICE_DATA) > + * at any time during the execution of the OS loader, > + * so we cannot consider them as safe. > + */ > + default: > + /* > + * Treat any other allocation in the region as unsafe */ > + status = EFI_OUT_OF_RESOURCES; > + goto out; > + } > + } > + > + status = EFI_SUCCESS; > +out: > + efi_call_early(free_pool, memory_map); > + return status; > +} > + > efi_status_t handle_kernel_image(efi_system_table_t *sys_table, > unsigned long *image_addr, > unsigned long *image_size, > @@ -71,10 +186,7 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, > unsigned long dram_base, > efi_loaded_image_t *image) > { > - unsigned long nr_pages; > efi_status_t status; > - /* Use alloc_addr to tranlsate between types */ > - efi_physical_addr_t alloc_addr; > > /* > * Verify that the DRAM base address is compatible with the ARM > @@ -85,27 +197,12 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, > */ > dram_base = round_up(dram_base, SZ_128M); > > - /* > - * Reserve memory for the uncompressed kernel image. This is > - * all that prevents any future allocations from conflicting > - * with the kernel. Since we can't tell from the compressed > - * image how much DRAM the kernel actually uses (due to BSS > - * size uncertainty) we allocate the maximum possible size. > - * Do this very early, as prints can cause memory allocations > - * that may conflict with this. > - */ > - alloc_addr = dram_base; > - *reserve_size = MAX_UNCOMP_KERNEL_SIZE; > - nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; > - status = sys_table->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, > - EFI_LOADER_DATA, > - nr_pages, &alloc_addr); > + status = reserve_kernel_base(sys_table, dram_base, reserve_addr, > + reserve_size); > if (status != EFI_SUCCESS) { > - *reserve_size = 0; > pr_efi_err(sys_table, "Unable to allocate memory for uncompressed kernel.\n"); > return status; > } > - *reserve_addr = alloc_addr; > > /* > * Relocate the zImage, so that it appears in the lowest 128 MB > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 March 2017 at 11:26, Leif Lindholm <leif.lindholm@linaro.org> 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@hp.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> >> 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. Yes, it does. But the alternative (assuming EfiLoaderData allocations in the region are safe) is worse, so I guess we will have to live with it. > >> drivers/firmware/efi/libstub/arm32-stub.c | 137 +++++++++++++++++--- >> 1 file changed, 117 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c >> index e1f0b28e1dcb..4e1b6478986e 100644 >> --- a/drivers/firmware/efi/libstub/arm32-stub.c >> +++ b/drivers/firmware/efi/libstub/arm32-stub.c >> @@ -63,6 +63,121 @@ void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) >> efi_call_early(free_pool, si); >> } >> >> +static efi_status_t reserve_kernel_base(efi_system_table_t *sys_table_arg, >> + unsigned long dram_base, >> + unsigned long *reserve_addr, >> + unsigned long *reserve_size) >> +{ >> + efi_physical_addr_t alloc_addr; >> + efi_memory_desc_t *memory_map; >> + unsigned long nr_pages, map_size, desc_size, buff_size; >> + efi_status_t status; >> + unsigned long l; >> + >> + struct efi_boot_memmap map = { >> + .map = &memory_map, >> + .map_size = &map_size, >> + .desc_size = &desc_size, >> + .desc_ver = NULL, >> + .key_ptr = NULL, >> + .buff_size = &buff_size, >> + }; >> + >> + /* >> + * Reserve memory for the uncompressed kernel image. This is >> + * all that prevents any future allocations from conflicting >> + * with the kernel. Since we can't tell from the compressed >> + * image how much DRAM the kernel actually uses (due to BSS >> + * size uncertainty) we allocate the maximum possible size. >> + * Do this very early, as prints can cause memory allocations >> + * that may conflict with this. >> + */ >> + alloc_addr = dram_base; >> + *reserve_size = MAX_UNCOMP_KERNEL_SIZE; >> + nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; >> + status = sys_table_arg->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, >> + EFI_BOOT_SERVICES_DATA, >> + nr_pages, &alloc_addr); >> + if (status == EFI_SUCCESS) { >> + *reserve_addr = alloc_addr; >> + return EFI_SUCCESS; >> + } >> + >> + /* >> + * If the allocation above failed, we may still be able to proceed: >> + * if the only allocations in the region are of types that will be >> + * released to the OS after ExitBootServices(), the decompressor can >> + * safely overwrite them. >> + */ >> + status = efi_get_memory_map(sys_table_arg, &map); >> + if (status != EFI_SUCCESS) { >> + pr_efi_err(sys_table_arg, >> + "reserve_kernel_base(): Unable to retrieve memory map.\n"); >> + return status; >> + } >> + >> + for (l = 0; l < map_size; l += desc_size) { >> + efi_memory_desc_t *desc; >> + u64 start, end; >> + >> + desc = (void *)memory_map + l; >> + start = desc->phys_addr; >> + end = start + desc->num_pages * EFI_PAGE_SIZE; >> + >> + /* does this entry cover the region? */ > > Nitpick: the logic tests the opposite of what the comment describes. > /* Skip if entry does not intersect with region */ > ? > > Anyway, that's minor. > I will change that before pushing. In any case, I'd like Eugene to confirm that this fixes the issue he reported before proceeding. > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. Nice find! > > That feels a little bit eeew, but I can't see it breaking anything. > > Yes, it does. But the alternative (assuming EfiLoaderData allocations in the > region are safe) is worse, so I guess we will have to live with it. The loader allocating boot service data regions is harmless enough since BS regions will effectively be owned by the OS/loader after ExitBootServices anyways. Reviewed-by: Eugene Cohen <eugene@hp.com>
On Thu, Mar 16, 2017 at 4:26 AM, Leif Lindholm <leif.lindholm@linaro.org> 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@hp.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> >> 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. I don't feel strongly about either of the above, as they are more theoretical problems. Reviewed-by: Roy Franz <roy.franz@cavium.com> >> drivers/firmware/efi/libstub/arm32-stub.c | 137 +++++++++++++++++--- >> 1 file changed, 117 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c >> index e1f0b28e1dcb..4e1b6478986e 100644 >> --- a/drivers/firmware/efi/libstub/arm32-stub.c >> +++ b/drivers/firmware/efi/libstub/arm32-stub.c >> @@ -63,6 +63,121 @@ void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) >> efi_call_early(free_pool, si); >> } >> >> +static efi_status_t reserve_kernel_base(efi_system_table_t *sys_table_arg, >> + unsigned long dram_base, >> + unsigned long *reserve_addr, >> + unsigned long *reserve_size) >> +{ >> + efi_physical_addr_t alloc_addr; >> + efi_memory_desc_t *memory_map; >> + unsigned long nr_pages, map_size, desc_size, buff_size; >> + efi_status_t status; >> + unsigned long l; >> + >> + struct efi_boot_memmap map = { >> + .map = &memory_map, >> + .map_size = &map_size, >> + .desc_size = &desc_size, >> + .desc_ver = NULL, >> + .key_ptr = NULL, >> + .buff_size = &buff_size, >> + }; >> + >> + /* >> + * Reserve memory for the uncompressed kernel image. This is >> + * all that prevents any future allocations from conflicting >> + * with the kernel. Since we can't tell from the compressed >> + * image how much DRAM the kernel actually uses (due to BSS >> + * size uncertainty) we allocate the maximum possible size. >> + * Do this very early, as prints can cause memory allocations >> + * that may conflict with this. >> + */ >> + alloc_addr = dram_base; >> + *reserve_size = MAX_UNCOMP_KERNEL_SIZE; >> + nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; >> + status = sys_table_arg->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, >> + EFI_BOOT_SERVICES_DATA, >> + nr_pages, &alloc_addr); This should use efi_call_early() to be consistent with the rest of the file. (It was the only direct call before, so we should fix that now.) Thanks, Roy >> + if (status == EFI_SUCCESS) { >> + *reserve_addr = alloc_addr; >> + return EFI_SUCCESS; >> + } >> + >> + /* >> + * If the allocation above failed, we may still be able to proceed: >> + * if the only allocations in the region are of types that will be >> + * released to the OS after ExitBootServices(), the decompressor can >> + * safely overwrite them. >> + */ >> + status = efi_get_memory_map(sys_table_arg, &map); >> + if (status != EFI_SUCCESS) { >> + pr_efi_err(sys_table_arg, >> + "reserve_kernel_base(): Unable to retrieve memory map.\n"); >> + return status; >> + } >> + >> + for (l = 0; l < map_size; l += desc_size) { >> + efi_memory_desc_t *desc; >> + u64 start, end; >> + >> + desc = (void *)memory_map + l; >> + start = desc->phys_addr; >> + end = start + desc->num_pages * EFI_PAGE_SIZE; >> + >> + /* does this entry cover the region? */ > > Nitpick: the logic tests the opposite of what the comment describes. > /* Skip if entry does not intersect with region */ > ? > > Anyway, that's minor. > > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > >> + if (start >= dram_base + MAX_UNCOMP_KERNEL_SIZE || >> + end <= dram_base) >> + continue; >> + >> + /* ignore types that are released to the OS anyway */ >> + switch (desc->type) { >> + case EFI_BOOT_SERVICES_CODE: >> + case EFI_BOOT_SERVICES_DATA: >> + /* these are safe -- ignore */ >> + continue; >> + >> + case EFI_CONVENTIONAL_MEMORY: >> + /* >> + * Reserve the intersection between this entry and the >> + * region. >> + */ >> + start = max(start, (u64)dram_base); >> + end = min(end, (u64)dram_base + MAX_UNCOMP_KERNEL_SIZE); >> + >> + status = efi_call_early(allocate_pages, >> + EFI_ALLOCATE_ADDRESS, >> + EFI_LOADER_DATA, >> + (end - start) / EFI_PAGE_SIZE, >> + &start); >> + if (status != EFI_SUCCESS) { >> + pr_efi_err(sys_table_arg, >> + "reserve_kernel_base(): alloc failed.\n"); >> + goto out; >> + } >> + break; >> + >> + case EFI_LOADER_CODE: >> + case EFI_LOADER_DATA: >> + /* >> + * These regions may be released and reallocated for >> + * another purpose (including EFI_RUNTIME_SERVICE_DATA) >> + * at any time during the execution of the OS loader, >> + * so we cannot consider them as safe. >> + */ >> + default: >> + /* >> + * Treat any other allocation in the region as unsafe */ >> + status = EFI_OUT_OF_RESOURCES; >> + goto out; >> + } >> + } >> + >> + status = EFI_SUCCESS; >> +out: >> + efi_call_early(free_pool, memory_map); >> + return status; >> +} >> + >> efi_status_t handle_kernel_image(efi_system_table_t *sys_table, >> unsigned long *image_addr, >> unsigned long *image_size, >> @@ -71,10 +186,7 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, >> unsigned long dram_base, >> efi_loaded_image_t *image) >> { >> - unsigned long nr_pages; >> efi_status_t status; >> - /* Use alloc_addr to tranlsate between types */ >> - efi_physical_addr_t alloc_addr; >> >> /* >> * Verify that the DRAM base address is compatible with the ARM >> @@ -85,27 +197,12 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, >> */ >> dram_base = round_up(dram_base, SZ_128M); >> >> - /* >> - * Reserve memory for the uncompressed kernel image. This is >> - * all that prevents any future allocations from conflicting >> - * with the kernel. Since we can't tell from the compressed >> - * image how much DRAM the kernel actually uses (due to BSS >> - * size uncertainty) we allocate the maximum possible size. >> - * Do this very early, as prints can cause memory allocations >> - * that may conflict with this. >> - */ >> - alloc_addr = dram_base; >> - *reserve_size = MAX_UNCOMP_KERNEL_SIZE; >> - nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; >> - status = sys_table->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, >> - EFI_LOADER_DATA, >> - nr_pages, &alloc_addr); >> + status = reserve_kernel_base(sys_table, dram_base, reserve_addr, >> + reserve_size); >> if (status != EFI_SUCCESS) { >> - *reserve_size = 0; >> pr_efi_err(sys_table, "Unable to allocate memory for uncompressed kernel.\n"); >> return status; >> } >> - *reserve_addr = alloc_addr; >> >> /* >> * Relocate the zImage, so that it appears in the lowest 128 MB >> -- >> 2.7.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 March 2017 at 21:56, Roy Franz <rfranz@cavium.com> wrote: > On Thu, Mar 16, 2017 at 4:26 AM, Leif Lindholm <leif.lindholm@linaro.org> 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@hp.com> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> >>> 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. > I don't feel strongly about either of the above, as they are more > theoretical problems. > > Reviewed-by: Roy Franz <roy.franz@cavium.com> >>> drivers/firmware/efi/libstub/arm32-stub.c | 137 +++++++++++++++++--- >>> 1 file changed, 117 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c >>> index e1f0b28e1dcb..4e1b6478986e 100644 >>> --- a/drivers/firmware/efi/libstub/arm32-stub.c >>> +++ b/drivers/firmware/efi/libstub/arm32-stub.c >>> @@ -63,6 +63,121 @@ void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) >>> efi_call_early(free_pool, si); >>> } >>> >>> +static efi_status_t reserve_kernel_base(efi_system_table_t *sys_table_arg, >>> + unsigned long dram_base, >>> + unsigned long *reserve_addr, >>> + unsigned long *reserve_size) >>> +{ >>> + efi_physical_addr_t alloc_addr; >>> + efi_memory_desc_t *memory_map; >>> + unsigned long nr_pages, map_size, desc_size, buff_size; >>> + efi_status_t status; >>> + unsigned long l; >>> + >>> + struct efi_boot_memmap map = { >>> + .map = &memory_map, >>> + .map_size = &map_size, >>> + .desc_size = &desc_size, >>> + .desc_ver = NULL, >>> + .key_ptr = NULL, >>> + .buff_size = &buff_size, >>> + }; >>> + >>> + /* >>> + * Reserve memory for the uncompressed kernel image. This is >>> + * all that prevents any future allocations from conflicting >>> + * with the kernel. Since we can't tell from the compressed >>> + * image how much DRAM the kernel actually uses (due to BSS >>> + * size uncertainty) we allocate the maximum possible size. >>> + * Do this very early, as prints can cause memory allocations >>> + * that may conflict with this. >>> + */ >>> + alloc_addr = dram_base; >>> + *reserve_size = MAX_UNCOMP_KERNEL_SIZE; >>> + nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; >>> + status = sys_table_arg->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, >>> + EFI_BOOT_SERVICES_DATA, >>> + nr_pages, &alloc_addr); > > This should use efi_call_early() to be consistent with the rest of the > file. (It was > the only direct call before, so we should fix that now.) > Good point, I will change that -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 16, 2017 at 3:02 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 16 March 2017 at 21:56, Roy Franz <rfranz@cavium.com> wrote: >> On Thu, Mar 16, 2017 at 4:26 AM, Leif Lindholm <leif.lindholm@linaro.org> 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@hp.com> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> --- >>>> >>>> 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. 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. Thanks, Roy > >> I don't feel strongly about either of the above, as they are more >> theoretical problems. >> >> Reviewed-by: Roy Franz <roy.franz@cavium.com> >>>> drivers/firmware/efi/libstub/arm32-stub.c | 137 +++++++++++++++++--- >>>> 1 file changed, 117 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c >>>> index e1f0b28e1dcb..4e1b6478986e 100644 >>>> --- a/drivers/firmware/efi/libstub/arm32-stub.c >>>> +++ b/drivers/firmware/efi/libstub/arm32-stub.c >>>> @@ -63,6 +63,121 @@ void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) >>>> efi_call_early(free_pool, si); >>>> } >>>> >>>> +static efi_status_t reserve_kernel_base(efi_system_table_t *sys_table_arg, >>>> + unsigned long dram_base, >>>> + unsigned long *reserve_addr, >>>> + unsigned long *reserve_size) >>>> +{ >>>> + efi_physical_addr_t alloc_addr; >>>> + efi_memory_desc_t *memory_map; >>>> + unsigned long nr_pages, map_size, desc_size, buff_size; >>>> + efi_status_t status; >>>> + unsigned long l; >>>> + >>>> + struct efi_boot_memmap map = { >>>> + .map = &memory_map, >>>> + .map_size = &map_size, >>>> + .desc_size = &desc_size, >>>> + .desc_ver = NULL, >>>> + .key_ptr = NULL, >>>> + .buff_size = &buff_size, >>>> + }; >>>> + >>>> + /* >>>> + * Reserve memory for the uncompressed kernel image. This is >>>> + * all that prevents any future allocations from conflicting >>>> + * with the kernel. Since we can't tell from the compressed >>>> + * image how much DRAM the kernel actually uses (due to BSS >>>> + * size uncertainty) we allocate the maximum possible size. >>>> + * Do this very early, as prints can cause memory allocations >>>> + * that may conflict with this. >>>> + */ >>>> + alloc_addr = dram_base; >>>> + *reserve_size = MAX_UNCOMP_KERNEL_SIZE; >>>> + nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; >>>> + status = sys_table_arg->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, >>>> + EFI_BOOT_SERVICES_DATA, >>>> + nr_pages, &alloc_addr); >> >> This should use efi_call_early() to be consistent with the rest of the >> file. (It was >> the only direct call before, so we should fix that now.) >> > > Good point, I will change that -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 March 2017 at 23:07, Roy Franz <rfranz@cavium.com> wrote: > On Thu, Mar 16, 2017 at 3:02 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 16 March 2017 at 21:56, Roy Franz <rfranz@cavium.com> wrote: >>> On Thu, Mar 16, 2017 at 4:26 AM, Leif Lindholm <leif.lindholm@linaro.org> 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@hp.com> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> --- >>>>> >>>>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c index e1f0b28e1dcb..4e1b6478986e 100644 --- a/drivers/firmware/efi/libstub/arm32-stub.c +++ b/drivers/firmware/efi/libstub/arm32-stub.c @@ -63,6 +63,121 @@ void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) efi_call_early(free_pool, si); } +static efi_status_t reserve_kernel_base(efi_system_table_t *sys_table_arg, + unsigned long dram_base, + unsigned long *reserve_addr, + unsigned long *reserve_size) +{ + efi_physical_addr_t alloc_addr; + efi_memory_desc_t *memory_map; + unsigned long nr_pages, map_size, desc_size, buff_size; + efi_status_t status; + unsigned long l; + + struct efi_boot_memmap map = { + .map = &memory_map, + .map_size = &map_size, + .desc_size = &desc_size, + .desc_ver = NULL, + .key_ptr = NULL, + .buff_size = &buff_size, + }; + + /* + * Reserve memory for the uncompressed kernel image. This is + * all that prevents any future allocations from conflicting + * with the kernel. Since we can't tell from the compressed + * image how much DRAM the kernel actually uses (due to BSS + * size uncertainty) we allocate the maximum possible size. + * Do this very early, as prints can cause memory allocations + * that may conflict with this. + */ + alloc_addr = dram_base; + *reserve_size = MAX_UNCOMP_KERNEL_SIZE; + nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; + status = sys_table_arg->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, + EFI_BOOT_SERVICES_DATA, + nr_pages, &alloc_addr); + if (status == EFI_SUCCESS) { + *reserve_addr = alloc_addr; + return EFI_SUCCESS; + } + + /* + * If the allocation above failed, we may still be able to proceed: + * if the only allocations in the region are of types that will be + * released to the OS after ExitBootServices(), the decompressor can + * safely overwrite them. + */ + status = efi_get_memory_map(sys_table_arg, &map); + if (status != EFI_SUCCESS) { + pr_efi_err(sys_table_arg, + "reserve_kernel_base(): Unable to retrieve memory map.\n"); + return status; + } + + for (l = 0; l < map_size; l += desc_size) { + efi_memory_desc_t *desc; + u64 start, end; + + desc = (void *)memory_map + l; + start = desc->phys_addr; + end = start + desc->num_pages * EFI_PAGE_SIZE; + + /* does this entry cover the region? */ + if (start >= dram_base + MAX_UNCOMP_KERNEL_SIZE || + end <= dram_base) + continue; + + /* ignore types that are released to the OS anyway */ + switch (desc->type) { + case EFI_BOOT_SERVICES_CODE: + case EFI_BOOT_SERVICES_DATA: + /* these are safe -- ignore */ + continue; + + case EFI_CONVENTIONAL_MEMORY: + /* + * Reserve the intersection between this entry and the + * region. + */ + start = max(start, (u64)dram_base); + end = min(end, (u64)dram_base + MAX_UNCOMP_KERNEL_SIZE); + + status = efi_call_early(allocate_pages, + EFI_ALLOCATE_ADDRESS, + EFI_LOADER_DATA, + (end - start) / EFI_PAGE_SIZE, + &start); + if (status != EFI_SUCCESS) { + pr_efi_err(sys_table_arg, + "reserve_kernel_base(): alloc failed.\n"); + goto out; + } + break; + + case EFI_LOADER_CODE: + case EFI_LOADER_DATA: + /* + * These regions may be released and reallocated for + * another purpose (including EFI_RUNTIME_SERVICE_DATA) + * at any time during the execution of the OS loader, + * so we cannot consider them as safe. + */ + default: + /* + * Treat any other allocation in the region as unsafe */ + status = EFI_OUT_OF_RESOURCES; + goto out; + } + } + + status = EFI_SUCCESS; +out: + efi_call_early(free_pool, memory_map); + return status; +} + efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, unsigned long *image_size, @@ -71,10 +186,7 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long dram_base, efi_loaded_image_t *image) { - unsigned long nr_pages; efi_status_t status; - /* Use alloc_addr to tranlsate between types */ - efi_physical_addr_t alloc_addr; /* * Verify that the DRAM base address is compatible with the ARM @@ -85,27 +197,12 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, */ dram_base = round_up(dram_base, SZ_128M); - /* - * Reserve memory for the uncompressed kernel image. This is - * all that prevents any future allocations from conflicting - * with the kernel. Since we can't tell from the compressed - * image how much DRAM the kernel actually uses (due to BSS - * size uncertainty) we allocate the maximum possible size. - * Do this very early, as prints can cause memory allocations - * that may conflict with this. - */ - alloc_addr = dram_base; - *reserve_size = MAX_UNCOMP_KERNEL_SIZE; - nr_pages = round_up(*reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; - status = sys_table->boottime->allocate_pages(EFI_ALLOCATE_ADDRESS, - EFI_LOADER_DATA, - nr_pages, &alloc_addr); + status = reserve_kernel_base(sys_table, dram_base, reserve_addr, + reserve_size); if (status != EFI_SUCCESS) { - *reserve_size = 0; pr_efi_err(sys_table, "Unable to allocate memory for uncompressed kernel.\n"); return status; } - *reserve_addr = alloc_addr; /* * Relocate the zImage, so that it appears in the lowest 128 MB
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@hp.com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- 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. drivers/firmware/efi/libstub/arm32-stub.c | 137 +++++++++++++++++--- 1 file changed, 117 insertions(+), 20 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html