On 16 March 2017 at 11:26, 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.
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@xxxxxxxxxx>
>
Thanks
--
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
