Fix stack canary handling (in the first patch) and re-index PVH GDT to
make it explicit that the GDT PVH-specific
v4:
* Load %gs after base address is calculated
* Increase stack canary segment size to 48 bytes for long 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]>
---
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 0169374..e7a5bf4 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]>
---
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..0169374 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 $canary, %rax
+ cdq
+ 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 22.05.18 at 05:54, <[email protected]> wrote:
> 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: Jan Beulich <[email protected]>
On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky
<[email protected]> wrote:
> 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 | 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..0169374 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 $canary, %rax
> + cdq
> + wrmsr
CDQ only sign-extends EAX to RAX. What you really want is to move the
high 32-bits to EDX (or zero EDX if we can guarantee it is loaded
below 4G).
--
Brian Gerst
>>> On 22.05.18 at 15:45, <[email protected]> wrote:
> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <[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 $canary, %rax
>> + cdq
>> + wrmsr
>
> CDQ only sign-extends EAX to RAX. What you really want is to move the
> high 32-bits to EDX (or zero EDX if we can guarantee it is loaded
> below 4G).
What you describe is CDQE (AT&T name: CLTD); CDQ (AT&T: CLTQ)
sign-extends EAX to EDX:EAX.
Jan
On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <[email protected]> wrote:
>>>> On 22.05.18 at 15:45, <[email protected]> wrote:
>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <[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 $canary, %rax
>>> + cdq
>>> + wrmsr
>>
>> CDQ only sign-extends EAX to RAX. What you really want is to move the
>> high 32-bits to EDX (or zero EDX if we can guarantee it is loaded
>> below 4G).
>
> What you describe is CDQE (AT&T name: CLTD); CDQ (AT&T: CLTQ)
> sign-extends EAX to EDX:EAX.
But that would still be wrong, as it would set EDX to 0xFFFFFFFF if
the kernel was loaded between 2G and 4G. Looking closer at the code,
we just left 32-bit mode, so we must have been loaded below 4G,
therefore EDX must be zero.
--
Brian Gerst
>>> On 22.05.18 at 17:15, <[email protected]> wrote:
> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <[email protected]> wrote:
>>>>> On 22.05.18 at 15:45, <[email protected]> wrote:
>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <[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 $canary, %rax
>>>> + cdq
>>>> + wrmsr
>>>
>>> CDQ only sign-extends EAX to RAX. What you really want is to move the
>>> high 32-bits to EDX (or zero EDX if we can guarantee it is loaded
>>> below 4G).
>>
>> What you describe is CDQE (AT&T name: CLTD); CDQ (AT&T: CLTQ)
>> sign-extends EAX to EDX:EAX.
>
> But that would still be wrong, as it would set EDX to 0xFFFFFFFF if
> the kernel was loaded between 2G and 4G. Looking closer at the code,
> we just left 32-bit mode, so we must have been loaded below 4G,
> therefore EDX must be zero.
Ah, yes, indeed.
Jan
On 05/22/2018 12:10 PM, Jan Beulich wrote:
>>>> On 22.05.18 at 17:15, <[email protected]> wrote:
>> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <[email protected]> wrote:
>>>>>> On 22.05.18 at 15:45, <[email protected]> wrote:
>>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <[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 $canary, %rax
>>>>> + cdq
>>>>> + wrmsr
>>>> CDQ only sign-extends EAX to RAX. What you really want is to move the
>>>> high 32-bits to EDX (or zero EDX if we can guarantee it is loaded
>>>> below 4G).
>>> What you describe is CDQE (AT&T name: CLTD); CDQ (AT&T: CLTQ)
>>> sign-extends EAX to EDX:EAX.
>> But that would still be wrong, as it would set EDX to 0xFFFFFFFF if
>> the kernel was loaded between 2G and 4G. Looking closer at the code,
>> we just left 32-bit mode, so we must have been loaded below 4G,
>> therefore EDX must be zero.
> Ah, yes, indeed.
We are loading virtual address for $canary so we will always have EDX
set to 0xffffffff. Isn't that what we want?
-borsi
>>> On 22.05.18 at 18:20, <[email protected]> wrote:
> On 05/22/2018 12:10 PM, Jan Beulich wrote:
>>>>> On 22.05.18 at 17:15, <[email protected]> wrote:
>>> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <[email protected]> wrote:
>>>>>>> On 22.05.18 at 15:45, <[email protected]> wrote:
>>>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <[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 $canary, %rax
>>>>>> + cdq
>>>>>> + wrmsr
>>>>> CDQ only sign-extends EAX to RAX. What you really want is to move the
>>>>> high 32-bits to EDX (or zero EDX if we can guarantee it is loaded
>>>>> below 4G).
>>>> What you describe is CDQE (AT&T name: CLTD); CDQ (AT&T: CLTQ)
>>>> sign-extends EAX to EDX:EAX.
>>> But that would still be wrong, as it would set EDX to 0xFFFFFFFF if
>>> the kernel was loaded between 2G and 4G. Looking closer at the code,
>>> we just left 32-bit mode, so we must have been loaded below 4G,
>>> therefore EDX must be zero.
>> Ah, yes, indeed.
>
> We are loading virtual address for $canary so we will always have EDX
> set to 0xffffffff. Isn't that what we want?
Oh, that's rather confusing - we're still running on the low 1:1
mapping when we're here. But yes, by the time we enter C code
(where the GS base starts to matter) we ought to be on the high
mappings - if only there wasn't xen_prepare_pvh().
Jan
On 05/22/2018 12:32 PM, Jan Beulich wrote:
>>>> On 22.05.18 at 18:20, <[email protected]> wrote:
>> On 05/22/2018 12:10 PM, Jan Beulich wrote:
>>>>>> On 22.05.18 at 17:15, <[email protected]> wrote:
>>>> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <[email protected]> wrote:
>>>>>>>> On 22.05.18 at 15:45, <[email protected]> wrote:
>>>>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <[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 $canary, %rax
>>>>>>> + cdq
>>>>>>> + wrmsr
>>>>>> CDQ only sign-extends EAX to RAX. What you really want is to move the
>>>>>> high 32-bits to EDX (or zero EDX if we can guarantee it is loaded
>>>>>> below 4G).
>>>>> What you describe is CDQE (AT&T name: CLTD); CDQ (AT&T: CLTQ)
>>>>> sign-extends EAX to EDX:EAX.
>>>> But that would still be wrong, as it would set EDX to 0xFFFFFFFF if
>>>> the kernel was loaded between 2G and 4G. Looking closer at the code,
>>>> we just left 32-bit mode, so we must have been loaded below 4G,
>>>> therefore EDX must be zero.
>>> Ah, yes, indeed.
>> We are loading virtual address for $canary so we will always have EDX
>> set to 0xffffffff. Isn't that what we want?
> Oh, that's rather confusing - we're still running on the low 1:1
> mapping when we're here. But yes, by the time we enter C code
> (where the GS base starts to matter) we ought to be on the high
> mappings - if only there wasn't xen_prepare_pvh().
xen_prepare_pvh() (and whatever it might call) is the only reason for
this patch to exist. It's the only C call that we are making before
jumping to startup_64, which I assume will have to set up GS itself
before calling into C.
I didn't realize we are still on identity mapping. I'll clear EDX (and
load $_pa(canary)) then.
BTW, don't we have the same issue in startup_xen()?
-boris
>>> On 22.05.18 at 19:10, <[email protected]> wrote:
> On 05/22/2018 12:32 PM, Jan Beulich wrote:
>>>>> On 22.05.18 at 18:20, <[email protected]> wrote:
>>> We are loading virtual address for $canary so we will always have EDX
>>> set to 0xffffffff. Isn't that what we want?
>> Oh, that's rather confusing - we're still running on the low 1:1
>> mapping when we're here. But yes, by the time we enter C code
>> (where the GS base starts to matter) we ought to be on the high
>> mappings - if only there wasn't xen_prepare_pvh().
>
> xen_prepare_pvh() (and whatever it might call) is the only reason for
> this patch to exist. It's the only C call that we are making before
> jumping to startup_64, which I assume will have to set up GS itself
> before calling into C.
>
> I didn't realize we are still on identity mapping. I'll clear EDX (and
> load $_pa(canary)) then.
>
> BTW, don't we have the same issue in startup_xen()?
I don't think so, no - there we're on the high mappings already (the
ELF note specifies the virtual address of the entry point, after all).
Jan
On 22/05/18 05:54, Boris Ostrovsky wrote:
> 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]>
With the clearing of EDX (instead using CDQ) you can add my
Reviewed-by: Juergen Gross <[email protected]>
Juergen
On 22/05/18 05:54, Boris Ostrovsky wrote:
> 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]>
Juergen