Re: [Xen-devel] [RFC 07/22] x86: relocate_kernel - Adapt assembly for PIE support

[Thomas Garnier <thgarnie@xxxxxxxxxx>]

On Wed, Jul 19, 2017 at 3:58 PM, H. Peter Anvin <hpa@xxxxxxxxx> wrote:
> On 07/18/17 15:33, Thomas Garnier wrote:
>> Change the assembly code to use only relative references of symbols for the
>> kernel to be PIE compatible.
>>
>> Position Independent Executable (PIE) support will allow to extended the
>> KASLR randomization range below the -2G memory limit.
>>
>> Signed-off-by: Thomas Garnier <thgarnie@xxxxxxxxxx>
>> ---
>>  arch/x86/kernel/relocate_kernel_64.S | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/relocate_kernel_64.S 
>> b/arch/x86/kernel/relocate_kernel_64.S
>> index 98111b38ebfd..da817d1628ac 100644
>> --- a/arch/x86/kernel/relocate_kernel_64.S
>> +++ b/arch/x86/kernel/relocate_kernel_64.S
>> @@ -186,7 +186,7 @@ identity_mapped:
>>       movq    %rax, %cr3
>>       lea     PAGE_SIZE(%r8), %rsp
>>       call    swap_pages
>> -     movq    $virtual_mapped, %rax
>> +     leaq    virtual_mapped(%rip), %rax
>>       pushq   %rax
>>       ret
>>
>
> This is completely wrong.  The whole point is that %rip here is on an
> identity-mapped page, which means that its offset to the actual symbol
> is ill-defined.
>
> The use of pushq/ret to do an indirect jump is bizarre, though, instead of:
>
>         pushq %r8
>         ret
>
> one ought to simply do
>
>         jmpq *%r8
>
> I think the author of this code was confused by the fact that we have to
> use this construct to do a *far* jump.
>
> There are some other very bizarre constructs in this file, that I can
> only assume comes from clumsy porting from 32 bits, for example:
>
>         call 1f
> 1:
>         popq %r8
>         subq $(1b - relocate_kernel), %r8
>
> ... instead of the much simpler ...
>
>         leaq relocate_kernel(%rip), %r8
>
> With this value in %r8 anyway, you can simply do:
>
>         leaq (virtual_mapped - relocate_kernel)(%r8), %rax
>         jmpq *%rax
>

Thanks I will look into that.

> This patchset scares me.  There seems to be a lot of places where you
> have not been very aware of what is actually happening in the code, nor
> have done research about how the ABIs actually work and affect things.

There is a lot of assembly that needed to be change. It was easier to
understand parts that are directly exercised like boot or percpu.
That's why I value people's feedback and will improve the patchset.

Thanks!

>
> Sorry.
>
>         -hpa



-- 
Thomas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel