LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 0/2] PVH GDT fixes
@ 2018-05-17 14:47 Boris Ostrovsky
  2018-05-17 14:47 ` [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
  2018-05-17 14:47 ` [PATCH v3 2/2] xen/PVH: Make GDT selectors PVH-specific Boris Ostrovsky
  0 siblings, 2 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2018-05-17 14:47 UTC (permalink / raw)
  To: xen-devel, linux-kernel; +Cc: jgross, JBeulich, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary
  2018-05-17 14:47 [PATCH v3 0/2] PVH GDT fixes Boris Ostrovsky
@ 2018-05-17 14:47 ` Boris Ostrovsky
  2018-05-17 15:02   ` Jan Beulich
  2018-05-17 14:47 ` [PATCH v3 2/2] xen/PVH: Make GDT selectors PVH-specific Boris Ostrovsky
  1 sibling, 1 reply; 6+ messages in thread
From: Boris Ostrovsky @ 2018-05-17 14:47 UTC (permalink / raw)
  To: xen-devel, linux-kernel; +Cc: jgross, JBeulich, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] xen/PVH: Make GDT selectors PVH-specific
  2018-05-17 14:47 [PATCH v3 0/2] PVH GDT fixes Boris Ostrovsky
  2018-05-17 14:47 ` [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
@ 2018-05-17 14:47 ` Boris Ostrovsky
  1 sibling, 0 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2018-05-17 14:47 UTC (permalink / raw)
  To: xen-devel, linux-kernel; +Cc: jgross, JBeulich, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary
  2018-05-17 14:47 ` [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
@ 2018-05-17 15:02   ` Jan Beulich
  2018-05-17 17:47     ` Boris Ostrovsky
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2018-05-17 15:02 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary
  2018-05-17 15:02   ` Jan Beulich
@ 2018-05-17 17:47     ` Boris Ostrovsky
  2018-05-18  7:31       ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Boris Ostrovsky @ 2018-05-17 17:47 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Juergen Gross, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary
  2018-05-17 17:47     ` Boris Ostrovsky
@ 2018-05-18  7:31       ` Jan Beulich
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2018-05-18  7:31 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-18  7:31 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-17 14:47 [PATCH v3 0/2] PVH GDT fixes Boris Ostrovsky
2018-05-17 14:47 ` [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
2018-05-17 15:02   ` Jan Beulich
2018-05-17 17:47     ` Boris Ostrovsky
2018-05-18  7:31       ` Jan Beulich
2018-05-17 14:47 ` [PATCH v3 2/2] xen/PVH: Make GDT selectors PVH-specific Boris Ostrovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).