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

2017-07-19 Thread Thomas Garnier 
On Wed, Jul 19, 2017 at 3:58 PM, H. Peter Anvin  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 
>> ---
>>  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
>>   callswap_pages
>> - movq$virtual_mapped, %rax
>> + leaqvirtual_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@lists.xen.org
https://lists.xen.org/xen-devel

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

2017-07-19 Thread H. Peter Anvin 
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 
> ---
>  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
>   callswap_pages
> - movq$virtual_mapped, %rax
> + leaqvirtual_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

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.

Sorry.

-hpa

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

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

2017-07-18 Thread Thomas Garnier 
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 
---
 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
callswap_pages
-   movq$virtual_mapped, %rax
+   leaqvirtual_mapped(%rip), %rax
pushq   %rax
ret
 
-- 
2.13.2.932.g7449e964c-goog


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