[PATCH v2] efi/arm64: handle missing virtual mapping for UEFI System Table

[Ard Biesheuvel]

(adding Catalin)

On 4 July 2014 16:42, Mark Salter <msalter at redhat.com> wrote:
> On Fri, 2014-07-04 at 12:16 +0200, Ard Biesheuvel wrote:
>> If we cannot resolve the virtual address of the UEFI System Table, its physical
>> offset must be missing from the virtual memory map, and there is really no point
>> in proceeding with installing the virtual memory map and the runtime services
>> dispatch table. So back out gracefully.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> ---
>>
>> v2:
>> - release mappings and free virtmap before bailing
>>
>>  arch/arm64/kernel/efi.c | 28 ++++++++++++++++++++++++++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index 56c3327bbf79..23942158e0f8 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -416,11 +416,23 @@ static int __init arm64_enter_virtual_mode(void)
>>                       continue;
>>               if (remap_region(md, &virt_md))
>>                       ++count;
>> +             else
>> +                     goto err_unmap;
>
> How about:
>                 if (!remap_region(md, &virt_md))
>                         goto err_unmap;
>                 ++count;
>
>

Sure.

>>       }
>>
>>       efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
>> -     if (efi.systab)
>> -             set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> +     if (!efi.systab) {
>> +             /*
>> +              * If we have no virtual mapping for the System Table at this
>> +              * point, the memory map doesn't cover the physical offset where
>> +              * it resides. This means the System Table will be inaccessible
>> +              * to Runtime Services themselves once the virtual mapping is
>> +              * installed.
>> +              */
>> +             pr_err("Failed to remap EFI System Table -- buggy firmware?\n");
>> +             goto err_unmap;
>> +     }
>> +     set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>>
>>       local_irq_save(flags);
>>       cpu_switch_mm(idmap_pg_dir, &init_mm);
>> @@ -453,5 +465,17 @@ static int __init arm64_enter_virtual_mode(void)
>>       set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>>
>>       return 0;
>> +
>> +err_unmap:
>> +     /* unmap all mappings that succeeded: there are 'count' of those */
>> +     for_each_efi_memory_desc(&memmap, md) {
>> +             if (!(md->attribute & EFI_MEMORY_RUNTIME))
>> +                     continue;
>> +             if (!count--)
>> +                     break;
>> +             iounmap((__force void *)md->virt_addr);
>> +     }
>
> This is wrong. memmap still belongs to UEFI and hasn't been touched. The
> new mappings are in virtmap. So, it is even simpler:
>
>         /* unmap all mappings that succeeded: there are 'count' of those */
>         for (virt_md = virtmap; count; virt_md++, count--)
>                 iounmap((__force void __iomem *)virt_md->virt_addr);
>

Yes, you're right. Will respin.

-- 
Ard.