[PATCH v3 0/2] PVH GDT fixes

[PATCH v3 0/2] PVH GDT fixes

[Boris Ostrovsky]

Fix stack canary handling (in the first patch) and re-index PVH GDT to
make it explicit that the GDT PVH-specific

v3:
- Use GS base MSR for 64-bit mode

Boris Ostrovsky (2):
  xen/PVH: Set up GS segment for stack canary
  xen/PVH: Make GDT selectors PVH-specific

 arch/x86/xen/xen-pvh.S | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

-- 
2.9.3

[PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

[Boris Ostrovsky]

We are making calls to C code (e.g. xen_prepare_pvh()) which may use
stack canary (stored in GS segment).

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/xen/xen-pvh.S | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index e1a5fbe..0db540c 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -54,6 +54,9 @@
  * charge of setting up it's own stack, GDT and IDT.
  */
 
+#define PVH_GDT_ENTRY_CANARY	4
+#define PVH_CANARY_SEL		(PVH_GDT_ENTRY_CANARY * 8)
+
 ENTRY(pvh_start_xen)
 	cld
 
@@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
 	mov %eax,%es
 	mov %eax,%ss
 
+	mov $PVH_CANARY_SEL,%eax
+	mov %eax,%gs
+
 	/* Stash hvm_start_info. */
 	mov $_pa(pvh_start_info), %edi
 	mov %ebx, %esi
@@ -98,6 +104,12 @@ ENTRY(pvh_start_xen)
 	/* 64-bit entry point. */
 	.code64
 1:
+	/* Set base address in stack canary descriptor. */
+	mov $MSR_GS_BASE,%ecx
+	mov $canary, %rax
+	cdq
+	wrmsr
+
 	call xen_prepare_pvh
 
 	/* startup_64 expects boot_params in %rsi. */
@@ -107,6 +119,14 @@ ENTRY(pvh_start_xen)
 
 #else /* CONFIG_X86_64 */
 
+	/* Set base address in stack canary descriptor. */
+	movl _pa(gdt_start),%eax
+	movl $_pa(canary),%ecx
+	movw %cx, (PVH_GDT_ENTRY_CANARY * 8) + 0(%eax)
+	shrl $16, %ecx
+	movb %cl, (PVH_GDT_ENTRY_CANARY * 8) + 2(%eax)
+	movb %ch, (PVH_GDT_ENTRY_CANARY * 8) + 5(%eax)
+
 	call mk_early_pgtbl_32
 
 	mov $_pa(initial_page_table), %eax
@@ -150,9 +170,12 @@ gdt_start:
 	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
 #endif
 	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* __KERNEL_DS */
+	.quad GDT_ENTRY(0x4090, 0, 0x18)    /* PVH_CANARY_SEL */
 gdt_end:
 
-	.balign 4
+	.balign 16
+canary:
+	.fill 24, 1, 0
 early_stack:
 	.fill 256, 1, 0
 early_stack_end:
-- 
2.9.3

[PATCH v3 2/2] xen/PVH: Make GDT selectors PVH-specific

[Boris Ostrovsky]

We don't need to share PVH GDT layout with other GDTs, especially
since we now have a PVH-speciific entry (for stack canary segment).

Define PVH's own selectors.

(As a side effect of this change we are also fixing improper
reference to __KERNEL_CS)

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/xen/xen-pvh.S | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index 0db540c..f09350a 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -54,7 +54,11 @@
  * charge of setting up it's own stack, GDT and IDT.
  */
 
-#define PVH_GDT_ENTRY_CANARY	4
+#define PVH_GDT_ENTRY_CS	1
+#define PVH_GDT_ENTRY_DS	2
+#define PVH_GDT_ENTRY_CANARY	3
+#define PVH_CS_SEL		(PVH_GDT_ENTRY_CS * 8)
+#define PVH_DS_SEL		(PVH_GDT_ENTRY_DS * 8)
 #define PVH_CANARY_SEL		(PVH_GDT_ENTRY_CANARY * 8)
 
 ENTRY(pvh_start_xen)
@@ -62,7 +66,7 @@ ENTRY(pvh_start_xen)
 
 	lgdt (_pa(gdt))
 
-	mov $(__BOOT_DS),%eax
+	mov $PVH_DS_SEL,%eax
 	mov %eax,%ds
 	mov %eax,%es
 	mov %eax,%ss
@@ -99,7 +103,7 @@ ENTRY(pvh_start_xen)
 	mov %eax, %cr0
 
 	/* Jump to 64-bit mode. */
-	ljmp $__KERNEL_CS, $_pa(1f)
+	ljmp $PVH_CS_SEL, $_pa(1f)
 
 	/* 64-bit entry point. */
 	.code64
@@ -136,13 +140,13 @@ ENTRY(pvh_start_xen)
 	or $(X86_CR0_PG | X86_CR0_PE), %eax
 	mov %eax, %cr0
 
-	ljmp $__BOOT_CS, $1f
+	ljmp $PVH_CS_SEL, $1f
 1:
 	call xen_prepare_pvh
 	mov $_pa(pvh_bootparams), %esi
 
 	/* startup_32 doesn't expect paging and PAE to be on. */
-	ljmp $__BOOT_CS, $_pa(2f)
+	ljmp $PVH_CS_SEL, $_pa(2f)
 2:
 	mov %cr0, %eax
 	and $~X86_CR0_PG, %eax
@@ -151,7 +155,7 @@ ENTRY(pvh_start_xen)
 	and $~X86_CR4_PAE, %eax
 	mov %eax, %cr4
 
-	ljmp $__BOOT_CS, $_pa(startup_32)
+	ljmp $PVH_CS_SEL, $_pa(startup_32)
 #endif
 END(pvh_start_xen)
 
@@ -163,13 +167,12 @@ gdt:
 	.word 0
 gdt_start:
 	.quad 0x0000000000000000            /* NULL descriptor */
-	.quad 0x0000000000000000            /* reserved */
 #ifdef CONFIG_X86_64
-	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
+	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* PVH_CS_SEL */
 #else
-	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
+	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* PVH_CS_SEL */
 #endif
-	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* __KERNEL_DS */
+	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* PVH_DS_SEL */
 	.quad GDT_ENTRY(0x4090, 0, 0x18)    /* PVH_CANARY_SEL */
 gdt_end:
 
-- 
2.9.3

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

[Jan Beulich]

>>> On 17.05.18 at 16:47, <boris.ostrovsky@oracle.com> wrote:
> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>  	mov %eax,%es
>  	mov %eax,%ss
>  
> +	mov $PVH_CANARY_SEL,%eax
> +	mov %eax,%gs

I doubt this is needed for 64-bit (you could equally well load zero or leave
in place what's there in that case), and loading the selector before setting
the base address in the descriptor won't have the intended effect.

> @@ -150,9 +170,12 @@ gdt_start:
>  	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
>  #endif
>  	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* __KERNEL_DS */
> +	.quad GDT_ENTRY(0x4090, 0, 0x18)    /* PVH_CANARY_SEL */
>  gdt_end:
>  
> -	.balign 4
> +	.balign 16
> +canary:
> +	.fill 24, 1, 0

This is too little space for 64-bit afaict (the canary lives at offset 40 there
if I can trust asm/processor.h).

Jan

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

[Boris Ostrovsky]

On 05/17/2018 11:02 AM, Jan Beulich wrote:
>>>> On 17.05.18 at 16:47, <boris.ostrovsky@oracle.com> wrote:
>> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>>  	mov %eax,%es
>>  	mov %eax,%ss
>>  
>> +	mov $PVH_CANARY_SEL,%eax
>> +	mov %eax,%gs
> I doubt this is needed for 64-bit (you could equally well load zero or leave
> in place what's there in that case),

I don't understand this.


>  and loading the selector before setting
> the base address in the descriptor won't have the intended effect.


I wasn't sure about this either but then I noticed that
secondary_startup_64() does it in the same order (although not using the
MSR).


>
>> @@ -150,9 +170,12 @@ gdt_start:
>>  	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
>>  #endif
>>  	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* __KERNEL_DS */
>> +	.quad GDT_ENTRY(0x4090, 0, 0x18)    /* PVH_CANARY_SEL */
>>  gdt_end:
>>  
>> -	.balign 4
>> +	.balign 16
>> +canary:
>> +	.fill 24, 1, 0
> This is too little space for 64-bit afaict (the canary lives at offset 40 there
> if I can trust asm/processor.h).

Yes, should be 48. I didn't realize the two modes use different offsets.

-boris

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

[Jan Beulich]

>>> On 17.05.18 at 19:47, <boris.ostrovsky@oracle.com> wrote:
> On 05/17/2018 11:02 AM, Jan Beulich wrote:
>>>>> On 17.05.18 at 16:47, <boris.ostrovsky@oracle.com> wrote:
>>> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>>>  	mov %eax,%es
>>>  	mov %eax,%ss
>>>  
>>> +	mov $PVH_CANARY_SEL,%eax
>>> +	mov %eax,%gs
>> I doubt this is needed for 64-bit (you could equally well load zero or leave
>> in place what's there in that case),
> 
> I don't understand this.

The actual selector value doesn't matter on 64-bit. Hence you could
omit the load altogether, or you could use plain zero. No need for the
(non-zero) selector, or (by implication) the GDT descriptor.

>>  and loading the selector before setting
>> the base address in the descriptor won't have the intended effect.
> 
> 
> I wasn't sure about this either but then I noticed that
> secondary_startup_64() does it in the same order (although not using the
> MSR).

Well, for one they load a null selector, which is independent of setting up
any GDT descriptors. I also don't understand why you say "although not
using the MSR" when they clearly do. And then, as said above (and also
in a comment in secondary_startup_64()), the actual selector value (and
when / if at all it is loaded) doesn't matter on 64-bit. The ordering does
matter on 32-bit though.

Jan