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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
>>> On 17.05.18 at 16:47, <[email protected]> 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
On 05/17/2018 11:02 AM, Jan Beulich wrote:
>>>> On 17.05.18 at 16:47, <[email protected]> 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
>>> On 17.05.18 at 19:47, <[email protected]> wrote:
> On 05/17/2018 11:02 AM, Jan Beulich wrote:
>>>>> On 17.05.18 at 16:47, <[email protected]> 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