Re: [Xen-devel] [RFC 06/22] kvm: Adapt assembly for PIE support

[H. Peter Anvin]

,Chris Metcalf ,"Paul E . 
McKenney" ,Andrew Morton 
,Christopher Li ,Dou Liyang 
,Masahiro Yamada 
,Daniel Borkmann ,Markus 
Trippelsdorf ,Peter Foley ,Steven 
Rostedt ,Tim Chen ,Catalin 
Marinas ,Matthew Wilcox 
,Michal Hocko ,Rob Landley 
,Jiri Kosina ,"H . J . Lu" 
,Paul Bolle ,Baoquan He 
,Daniel Micay ,the arch/x86 maintainers 
,"linux-cry...@vger.kernel.org" 
,Linux Kernel Mailing List 
,xen-de...@lists.xenproject.org,kvm list
,linux-pm ,linux-arch 
,Linux-Sparse ,Kernel 
Hardening 
From: h...@zytor.com
Message-ID: <83ba7600-bc8d-4c91-812c-dd2a0bf44...@zytor.com>

On July 19, 2017 3:58:07 PM PDT, Ard Biesheuvel  
wrote:
>On 19 July 2017 at 23:27, H. Peter Anvin  wrote:
>> On 07/19/17 08:40, Thomas Garnier wrote:

 This doesn't look right.  It's accessing a per-cpu variable.  The
 per-cpu section is an absolute, zero-based section and not subject
>to
 relocation.
>>>
>>> PIE does not respect the zero-based section, it tries to have
>>> everything relative. Patch 16/22 also adapt per-cpu to work with PIE
>>> (while keeping the zero absolute design by default).
>>>
>>
>> This is silly.  The right thing is for PIE is to be explicitly
>absolute,
>> without (%rip).  The use of (%rip) memory references for percpu is
>just
>> an optimization.
>>
>
>Sadly, there is an issue in binutils that may prevent us from doing
>this as cleanly as we would want.
>
>For historical reasons, bfd.ld emits special symbols like
>__GLOBAL_OFFSET_TABLE__ as absolute symbols with a section index of
>SHN_ABS, even though it is quite obvious that they are relative like
>any other symbol that points into the image. Unfortunately, this means
>that binutils needs to emit R_X86_64_RELATIVE relocations even for
>SHN_ABS symbols, which means we lose the ability to use both absolute
>and relocatable symbols in the same PIE image (unless the reloc tool
>can filter them out)
>
>More info here:
>https://sourceware.org/bugzilla/show_bug.cgi?id=19818

The reloc tool already has the ability to filter symbols.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

Re: [Xen-devel] [RFC 06/22] kvm: Adapt assembly for PIE support

[Ard Biesheuvel]

On 19 July 2017 at 23:27, H. Peter Anvin  wrote:
> On 07/19/17 08:40, Thomas Garnier wrote:
>>>
>>> This doesn't look right.  It's accessing a per-cpu variable.  The
>>> per-cpu section is an absolute, zero-based section and not subject to
>>> relocation.
>>
>> PIE does not respect the zero-based section, it tries to have
>> everything relative. Patch 16/22 also adapt per-cpu to work with PIE
>> (while keeping the zero absolute design by default).
>>
>
> This is silly.  The right thing is for PIE is to be explicitly absolute,
> without (%rip).  The use of (%rip) memory references for percpu is just
> an optimization.
>

Sadly, there is an issue in binutils that may prevent us from doing
this as cleanly as we would want.

For historical reasons, bfd.ld emits special symbols like
__GLOBAL_OFFSET_TABLE__ as absolute symbols with a section index of
SHN_ABS, even though it is quite obvious that they are relative like
any other symbol that points into the image. Unfortunately, this means
that binutils needs to emit R_X86_64_RELATIVE relocations even for
SHN_ABS symbols, which means we lose the ability to use both absolute
and relocatable symbols in the same PIE image (unless the reloc tool
can filter them out)

More info here:
https://sourceware.org/bugzilla/show_bug.cgi?id=19818

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

Re: [Xen-devel] [RFC 06/22] kvm: Adapt assembly for PIE support

[H. Peter Anvin]

On 07/19/17 08:40, Thomas Garnier wrote:
>>
>> This doesn't look right.  It's accessing a per-cpu variable.  The
>> per-cpu section is an absolute, zero-based section and not subject to
>> relocation.
> 
> PIE does not respect the zero-based section, it tries to have
> everything relative. Patch 16/22 also adapt per-cpu to work with PIE
> (while keeping the zero absolute design by default).
> 

This is silly.  The right thing is for PIE is to be explicitly absolute,
without (%rip).  The use of (%rip) memory references for percpu is just
an optimization.

-hpa


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

Re: [Xen-devel] [RFC 06/22] kvm: Adapt assembly for PIE support

[Thomas Garnier]

On Wed, Jul 19, 2017 at 3:27 PM, H. Peter Anvin  wrote:
> On 07/19/17 08:40, Thomas Garnier wrote:
>>>
>>> This doesn't look right.  It's accessing a per-cpu variable.  The
>>> per-cpu section is an absolute, zero-based section and not subject to
>>> relocation.
>>
>> PIE does not respect the zero-based section, it tries to have
>> everything relative. Patch 16/22 also adapt per-cpu to work with PIE
>> (while keeping the zero absolute design by default).
>>
>
> This is silly.  The right thing is for PIE is to be explicitly absolute,
> without (%rip).  The use of (%rip) memory references for percpu is just
> an optimization.

I agree that it is odd but that's how the compiler generates code. I
will re-explore PIC options with mcmodel=small or medium, as mentioned
on other threads.

>
> -hpa
>



-- 
Thomas

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

Re: [Xen-devel] [RFC 06/22] kvm: Adapt assembly for PIE support

[Thomas Garnier]

On Tue, Jul 18, 2017 at 7:49 PM, Brian Gerst  wrote:
> On Tue, Jul 18, 2017 at 6:33 PM, Thomas Garnier  wrote:
>> Change the assembly code to use only relative references of symbols for the
>> kernel to be PIE compatible. The new __ASM_GET_PTR_PRE macro is used to
>> get the address of a symbol on both 32 and 64-bit with PIE support.
>>
>> 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/include/asm/kvm_host.h | 6 --
>>  arch/x86/kernel/kvm.c   | 6 --
>>  arch/x86/kvm/svm.c  | 4 ++--
>>  3 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h 
>> b/arch/x86/include/asm/kvm_host.h
>> index 87ac4fba6d8e..3041201a3aeb 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1352,9 +1352,11 @@ asmlinkage void kvm_spurious_fault(void);
>> ".pushsection .fixup, \"ax\" \n" \
>> "667: \n\t" \
>> cleanup_insn "\n\t"   \
>> -   "cmpb $0, kvm_rebooting \n\t" \
>> +   "cmpb $0, kvm_rebooting" __ASM_SEL(,(%%rip)) " \n\t" \
>> "jne 668b \n\t"   \
>> -   __ASM_SIZE(push) " $666b \n\t"\
>> +   __ASM_SIZE(push) "%%" _ASM_AX " \n\t"   \
>> +   __ASM_GET_PTR_PRE(666b) "%%" _ASM_AX "\n\t" \
>> +   "xchg %%" _ASM_AX ", (%%" _ASM_SP ") \n\t"  \
>> "call kvm_spurious_fault \n\t"\
>> ".popsection \n\t" \
>> _ASM_EXTABLE(666b, 667b)
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 71c17a5be983..53b8ad162589 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -618,8 +618,10 @@ asm(
>>  ".global __raw_callee_save___kvm_vcpu_is_preempted;"
>>  ".type __raw_callee_save___kvm_vcpu_is_preempted, @function;"
>>  "__raw_callee_save___kvm_vcpu_is_preempted:"
>> -"movq  __per_cpu_offset(,%rdi,8), %rax;"
>> -"cmpb  $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);"
>> +"leaq  __per_cpu_offset(%rip), %rax;"
>> +"movq  (%rax,%rdi,8), %rax;"
>> +"addq  " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rip), %rax;"
>
> This doesn't look right.  It's accessing a per-cpu variable.  The
> per-cpu section is an absolute, zero-based section and not subject to
> relocation.
>

PIE does not respect the zero-based section, it tries to have
everything relative. Patch 16/22 also adapt per-cpu to work with PIE
(while keeping the zero absolute design by default).

>> +"cmpb  $0, (%rax);
>>  "setne %al;"
>>  "ret;"
>>  ".popsection");
>
> --
> Brian Gerst



-- 
Thomas

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

Re: [Xen-devel] [RFC 06/22] kvm: Adapt assembly for PIE support

[Brian Gerst]

On Tue, Jul 18, 2017 at 6:33 PM, Thomas Garnier  wrote:
> Change the assembly code to use only relative references of symbols for the
> kernel to be PIE compatible. The new __ASM_GET_PTR_PRE macro is used to
> get the address of a symbol on both 32 and 64-bit with PIE support.
>
> 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/include/asm/kvm_host.h | 6 --
>  arch/x86/kernel/kvm.c   | 6 --
>  arch/x86/kvm/svm.c  | 4 ++--
>  3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 87ac4fba6d8e..3041201a3aeb 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1352,9 +1352,11 @@ asmlinkage void kvm_spurious_fault(void);
> ".pushsection .fixup, \"ax\" \n" \
> "667: \n\t" \
> cleanup_insn "\n\t"   \
> -   "cmpb $0, kvm_rebooting \n\t" \
> +   "cmpb $0, kvm_rebooting" __ASM_SEL(,(%%rip)) " \n\t" \
> "jne 668b \n\t"   \
> -   __ASM_SIZE(push) " $666b \n\t"\
> +   __ASM_SIZE(push) "%%" _ASM_AX " \n\t"   \
> +   __ASM_GET_PTR_PRE(666b) "%%" _ASM_AX "\n\t" \
> +   "xchg %%" _ASM_AX ", (%%" _ASM_SP ") \n\t"  \
> "call kvm_spurious_fault \n\t"\
> ".popsection \n\t" \
> _ASM_EXTABLE(666b, 667b)
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 71c17a5be983..53b8ad162589 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -618,8 +618,10 @@ asm(
>  ".global __raw_callee_save___kvm_vcpu_is_preempted;"
>  ".type __raw_callee_save___kvm_vcpu_is_preempted, @function;"
>  "__raw_callee_save___kvm_vcpu_is_preempted:"
> -"movq  __per_cpu_offset(,%rdi,8), %rax;"
> -"cmpb  $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);"
> +"leaq  __per_cpu_offset(%rip), %rax;"
> +"movq  (%rax,%rdi,8), %rax;"
> +"addq  " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rip), %rax;"

This doesn't look right.  It's accessing a per-cpu variable.  The
per-cpu section is an absolute, zero-based section and not subject to
relocation.

> +"cmpb  $0, (%rax);
>  "setne %al;"
>  "ret;"
>  ".popsection");

--
Brian Gerst

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

[Xen-devel] [RFC 06/22] kvm: Adapt assembly for PIE support

[Thomas Garnier]

Change the assembly code to use only relative references of symbols for the
kernel to be PIE compatible. The new __ASM_GET_PTR_PRE macro is used to
get the address of a symbol on both 32 and 64-bit with PIE support.

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/include/asm/kvm_host.h | 6 --
 arch/x86/kernel/kvm.c   | 6 --
 arch/x86/kvm/svm.c  | 4 ++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 87ac4fba6d8e..3041201a3aeb 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1352,9 +1352,11 @@ asmlinkage void kvm_spurious_fault(void);
".pushsection .fixup, \"ax\" \n" \
"667: \n\t" \
cleanup_insn "\n\t"   \
-   "cmpb $0, kvm_rebooting \n\t" \
+   "cmpb $0, kvm_rebooting" __ASM_SEL(,(%%rip)) " \n\t" \
"jne 668b \n\t"   \
-   __ASM_SIZE(push) " $666b \n\t"\
+   __ASM_SIZE(push) "%%" _ASM_AX " \n\t"   \
+   __ASM_GET_PTR_PRE(666b) "%%" _ASM_AX "\n\t" \
+   "xchg %%" _ASM_AX ", (%%" _ASM_SP ") \n\t"  \
"call kvm_spurious_fault \n\t"\
".popsection \n\t" \
_ASM_EXTABLE(666b, 667b)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 71c17a5be983..53b8ad162589 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -618,8 +618,10 @@ asm(
 ".global __raw_callee_save___kvm_vcpu_is_preempted;"
 ".type __raw_callee_save___kvm_vcpu_is_preempted, @function;"
 "__raw_callee_save___kvm_vcpu_is_preempted:"
-"movq  __per_cpu_offset(,%rdi,8), %rax;"
-"cmpb  $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);"
+"leaq  __per_cpu_offset(%rip), %rax;"
+"movq  (%rax,%rdi,8), %rax;"
+"addq  " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rip), %rax;"
+"cmpb  $0, (%rax);"
 "setne %al;"
 "ret;"
 ".popsection");
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4d8141e533c3..8b718c6d6729 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -554,12 +554,12 @@ static u32 svm_msrpm_offset(u32 msr)
 
 static inline void clgi(void)
 {
-   asm volatile (__ex(SVM_CLGI));
+   asm volatile (__ex(SVM_CLGI) : :);
 }
 
 static inline void stgi(void)
 {
-   asm volatile (__ex(SVM_STGI));
+   asm volatile (__ex(SVM_STGI) : :);
 }
 
 static inline void invlpga(unsigned long addr, u32 asid)
-- 
2.13.2.932.g7449e964c-goog


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