Fix stack canary handling (in the first patch) and re-index PVH GDT to
make it explicit that the GDT PVH-specific
v5:
- Load canary's physical address and clear %edx 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 | 47 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 37 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]>
Reviewed-by: Juergen Gross <[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 d6a17b9..dd852ac 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
@@ -96,7 +100,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]>
Reviewed-by: Juergen Gross <[email protected]>
---
arch/x86/xen/xen-pvh.S | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index e1a5fbe..d6a17b9 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
@@ -98,6 +101,12 @@ ENTRY(pvh_start_xen)
/* 64-bit entry point. */
.code64
1:
+ /* Set base address in stack canary descriptor. */
+ mov $MSR_GS_BASE,%ecx
+ mov $_pa(canary), %rax
+ xor %rdx, %rdx
+ wrmsr
+
call xen_prepare_pvh
/* startup_64 expects boot_params in %rsi. */
@@ -107,6 +116,17 @@ 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) + 2(%eax)
+ shrl $16, %ecx
+ movb %cl, (PVH_GDT_ENTRY_CANARY * 8) + 4(%eax)
+ movb %ch, (PVH_GDT_ENTRY_CANARY * 8) + 7(%eax)
+
+ mov $PVH_CANARY_SEL,%eax
+ mov %eax,%gs
+
call mk_early_pgtbl_32
mov $_pa(initial_page_table), %eax
@@ -150,9 +170,13 @@ 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 48, 1, 0
+
early_stack:
.fill 256, 1, 0
early_stack_end:
--
2.9.3
>>> On 23.05.18 at 16:30, <[email protected]> wrote:
> @@ -98,6 +101,12 @@ ENTRY(pvh_start_xen)
> /* 64-bit entry point. */
> .code64
> 1:
> + /* Set base address in stack canary descriptor. */
> + mov $MSR_GS_BASE,%ecx
> + mov $_pa(canary), %rax
> + xor %rdx, %rdx
Why rax and rdx instead of eax and edx? In the former case, the
relocation produced might confuse whatever entity processing it
(it'll have a sign-extended 32-bit quantity to deal with, which
wouldn't allow representing an address in the [2Gb, 4Gb) range).
In the latter case, while surely neither performance nor code size
matter much here, it's still a bad precedent (people copy-and-paste
code all the time): Zero-ing of registers should generally use the
32-bit forms of the insn. Gas has actually gained an optimization
mode recently (upon request from Linus and the x86 maintainers)
to silently "repair" such inefficiencies.
Jan
On 05/23/2018 11:41 AM, Jan Beulich wrote:
>>>> On 23.05.18 at 16:30, <[email protected]> wrote:
>> @@ -98,6 +101,12 @@ ENTRY(pvh_start_xen)
>> /* 64-bit entry point. */
>> .code64
>> 1:
>> + /* Set base address in stack canary descriptor. */
>> + mov $MSR_GS_BASE,%ecx
>> + mov $_pa(canary), %rax
>> + xor %rdx, %rdx
> Why rax and rdx instead of eax and edx? In the former case, the
> relocation produced might confuse whatever entity processing it
> (it'll have a sign-extended 32-bit quantity to deal with, which
> wouldn't allow representing an address in the [2Gb, 4Gb) range).
> In the latter case, while surely neither performance nor code size
> matter much here, it's still a bad precedent (people copy-and-paste
> code all the time): Zero-ing of registers should generally use the
> 32-bit forms of the insn. Gas has actually gained an optimization
> mode recently (upon request from Linus and the x86 maintainers)
> to silently "repair" such inefficiencies.
Sure, I can replace these two with 32-bit variants. If there are no
other comments I won't re-send this again.
-boris