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

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

[Julien Grall]

The EFI image should be relocatable. At the moment, all the stub is
relocatable but one place.

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.

For instance, at least on Arm64, EFI will do a 1:1 mappings of the Stub.

Arguably this is either a compiler bug or a problem with the flags
passed. I narrow down the problem to the following snippet:

const char * const ErrCodeToStr[] = {
    "Not found",
    "The device has no media",
};

const char * foo(unsigned int i)
{
    return ErrCodeToStr[i];
}

To prevent the compiler using absolute pointer, specify the maximum size
of the string.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---

I am not entirely convinced this is the right way. But I though I would
start a conversation to get feedback.
---
 xen/common/efi/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 469bf980cc..87d46f2a56 100644
--- 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",
-- 
2.11.0


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

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

[Jan Beulich]

>>> 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 ...

> 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.

> 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. 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)?

> --- 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.

Jan


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

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

[Julien Grall]

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

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

[Jan Beulich]

>>> On 09.01.18 at 20:43, <julien.grall@linaro.org> wrote:
> 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.

I can see them there in the binary I look at. I use my own tool
for dumping, so the output may look unfamiliar to you, but here
are the relevant pieces:

Section count                               0009 (         9)
...
  Section 0004 (    4): '.init', size 00085578, RVA 00600000, flags E0D00060
  code data read write exec 4096-byte align 
    00086000 bytes at file offset 00191000

Symbol count                            00001DE2 (      7650)
...
  0000115A: ErrCodeToStr.9795
    Value 00044340, Section 0004, Type none, Storage static

which makes the array start at RVA 0x644340.

 Fixups for page at 00644000, 00000094 (       148) bytes
...
  DIR64   0318  DIR64   0320  DIR64   0328  DIR64   0368  DIR64   0378
  DIR64   0388  DIR64   0390  DIR64   0398  DIR64   03A0  DIR64   03A8
  DIR64   03B0  DIR64   03B8  DIR64   0410  DIR64   0418  DIR64   0448

The numbers here are the offsets into the page named in the
"title" line, and the last 12 are the ones targeting the array in
question.

> 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.

Well, without having seen the binary I don't think I can conclude
in the direction of compiler bug. Please don't forget that ld itself
does indeed not (yet) create any relocations in PE executables
(which an EFI application is). They're being added in a post-
processing step (hence the need to link the binary twice at
different base addresses, for the helper tool [mkreloc] to figure
out where relocations are needed).

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

Sure, but an error path is being taken every now and then, and
I personally have seen errors coming back (mostly after having
made mistakes elsewhere).

Jan


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

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

[Julien Grall]

Hi Jan,

On 12/01/18 13:13, Jan Beulich wrote:
>>>> On 09.01.18 at 20:43, <julien.grall@linaro.org> wrote:
>> 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.
> 
> I can see them there in the binary I look at. I use my own tool
> for dumping, so the output may look unfamiliar to you, but here
> are the relevant pieces:

I am a bit confused. Which binary are you looking at? Is it xen.efi?
I don't seem to find them in xen-syms.

> 
> Section count                               0009 (         9)
> ...
>    Section 0004 (    4): '.init', size 00085578, RVA 00600000, flags E0D00060
>    code data read write exec 4096-byte align
>      00086000 bytes at file offset 00191000
> 
> Symbol count                            00001DE2 (      7650)
> ...
>    0000115A: ErrCodeToStr.9795
>      Value 00044340, Section 0004, Type none, Storage static
> 
> which makes the array start at RVA 0x644340.
> 
>   Fixups for page at 00644000, 00000094 (       148) bytes
> ...
>    DIR64   0318  DIR64   0320  DIR64   0328  DIR64   0368  DIR64   0378
>    DIR64   0388  DIR64   0390  DIR64   0398  DIR64   03A0  DIR64   03A8
>    DIR64   03B0  DIR64   03B8  DIR64   0410  DIR64   0418  DIR64   0448
> 
> The numbers here are the offsets into the page named in the
> "title" line, and the last 12 are the ones targeting the array in
> question.
> 
>> 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.
> 
> Well, without having seen the binary I don't think I can conclude
> in the direction of compiler bug. Please don't forget that ld itself
> does indeed not (yet) create any relocations in PE executables
> (which an EFI application is). They're being added in a post-
> processing step (hence the need to link the binary twice at
> different base addresses, for the helper tool [mkreloc] to figure
> out where relocations are needed).

If the code compiled is position independent, then you should not need
relocation. Right? So what are you trying to achieve with mkreloc?

Also, it does not explain why the compiler is issuing absolute address 
when building with -fpie.

> 
>> So I am wondering how this work on x86? Note that this code is only used
>> in error path.
> 
> Sure, but an error path is being taken every now and then, and
> I personally have seen errors coming back (mostly after having
> made mistakes elsewhere).

And I guess the binary will never be loaded at the same as virtual 
address as Xen would be meant to run?

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

>>> On 16.01.18 at 18:43, <julien.grall@linaro.org> wrote:
> On 12/01/18 13:13, Jan Beulich wrote:
>>>>> On 09.01.18 at 20:43, <julien.grall@linaro.org> wrote:
>>> 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.
>> 
>> I can see them there in the binary I look at. I use my own tool
>> for dumping, so the output may look unfamiliar to you, but here
>> are the relevant pieces:
> 
> I am a bit confused. Which binary are you looking at? Is it xen.efi?
> I don't seem to find them in xen-syms.

Of course it is xen.efi. xen-syms can't possibly have any PE relocations,
as it's an ELF image on x86.

>> Well, without having seen the binary I don't think I can conclude
>> in the direction of compiler bug. Please don't forget that ld itself
>> does indeed not (yet) create any relocations in PE executables
>> (which an EFI application is). They're being added in a post-
>> processing step (hence the need to link the binary twice at
>> different base addresses, for the helper tool [mkreloc] to figure
>> out where relocations are needed).
> 
> If the code compiled is position independent, then you should not need
> relocation. Right? So what are you trying to achieve with mkreloc?

If _all_ of the code, even assembly, was position independent,
then yes, there shouldn't be relocations. But that's not the case
right now.

> Also, it does not explain why the compiler is issuing absolute address 
> when building with -fpie.

To be honest I'm not certain what guarantees -fpie makes on the
compiled code. It looks to me as if it only tries to minimize the
relocations needed, not eliminate all of them (after all the binary
will work fine that way, it is just required that the relocations not
be removed while linking the final binary). Indeed both 32-bit and
64-bit for both x86 and ARM produce relocations for tables like
the one we talk about (so called absolute ones on ARM), yet I
don't think this is a compiler bug.

>>> So I am wondering how this work on x86? Note that this code is only used
>>> in error path.
>> 
>> Sure, but an error path is being taken every now and then, and
>> I personally have seen errors coming back (mostly after having
>> made mistakes elsewhere).
> 
> And I guess the binary will never be loaded at the same as virtual 
> address as Xen would be meant to run?

Indeed - it can't be loaded at that address, as the 1:1 mapping the
EFI environment requires can't ever have any present mappings in
the upper half of the virtual address space.

Jan


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