Re: [PATCH] xen/efi: Avoid EFI stub using absolute symbols

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 01/02/2018 04:35 PM, Jan Beulich wrote:
>>>> On 21.12.17 at 15:55, <julien.grall@linaro.org> wrote:
>> The EFI image should be relocatable. At the moment, all the stub is
>> relocatable but one place.
> 
> Do you really mean relocatable here? Based on ...

Hmm yes position independent.

> 
>> On both Arm64 and x86-64 (from a quick glance) , the compiler will generate
>> absolute pointer in the ErrCodeToStr array. Those values are based on Xen
>> view of the virtual memory and may not be the same as EFI.
> 
> ... this I'm wondering whether you don't instead mean position
> independent. xen.efi (on x86) wouldn't work right if there were
> no relocations for this array.

When I compiled the snippet on x86 and Arm, no relocation is available 
for the pointers to string in the array in the final binary. Yet they 
are available in the object.

Indeed the relocation seem to be absolute (e.g R_X86_64_64) and 
disappeared at linking. Hence why I suggested a compiler bug because the 
code should be PIE and that would not even work is the binary is 
randomized on Linux.

So I am wondering how this work on x86? Note that this code is only used 
in error path.

> 
>> For instance, at least on Arm64, EFI will do a 1:1 mappings of the Stub.
> 
> I'm afraid it is not clear to me what "1:1 mapping" in this context
> means.

I meant VA = PA.

> Isn't your problem rather than on ARM64 xen.efi's
> .reloc section is empty (which presumably is a result of how it is
> being built)?

See above.

> 
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -342,7 +342,7 @@ static void __init noreturn blexit(const CHAR16 *str)
>>   /* generic routine for printing error messages */
>>   static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
>>   {
>> -    static const CHAR16* const ErrCodeToStr[] __initconstrel = {
>> +    static const CHAR16 ErrCodeToStr[][25] __initconst  = {
>>           [~EFI_ERROR_MASK & EFI_NOT_FOUND]           = L"Not found",
>>           [~EFI_ERROR_MASK & EFI_NO_MEDIA]            = L"The device has no media",
>>           [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED]       = L"Media changed",
> 
> If we really wanted (needed) to go this route, at least a comment
> would be needed to prevent someone later "correcting" to obvious
> oddity by switching back to what we have now. But I'd prefer if this
> code was left alone.

That my preferred way too. But at the moment, I can't see how to avoid 
leave the array unchanged.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel