[PATCH v4 0/2] PVH GDT fixes

[PATCH v4 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

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

[PATCH v4 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 | 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

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

[Jan Beulich]

>>> On 22.05.18 at 05:54, <boris.ostrovsky@oracle.com> 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 <boris.ostrovsky@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Brian Gerst]

On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> 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 <boris.ostrovsky@oracle.com>
> ---
>  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

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

[Jan Beulich]

>>> On 22.05.18 at 15:45, <brgerst@gmail.com> wrote:
> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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

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

[Brian Gerst]

On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.05.18 at 15:45, <brgerst@gmail.com> wrote:
>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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

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

[Jan Beulich]

>>> On 22.05.18 at 17:15, <brgerst@gmail.com> wrote:
> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.05.18 at 15:45, <brgerst@gmail.com> wrote:
>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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

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

[Boris Ostrovsky]

On 05/22/2018 12:10 PM, Jan Beulich wrote:
>>>> On 22.05.18 at 17:15, <brgerst@gmail.com> wrote:
>> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 22.05.18 at 15:45, <brgerst@gmail.com> wrote:
>>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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

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

[Jan Beulich]

>>> On 22.05.18 at 18:20, <boris.ostrovsky@oracle.com> wrote:
> On 05/22/2018 12:10 PM, Jan Beulich wrote:
>>>>> On 22.05.18 at 17:15, <brgerst@gmail.com> wrote:
>>> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 22.05.18 at 15:45, <brgerst@gmail.com> wrote:
>>>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com> 
> 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

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

[Boris Ostrovsky]

On 05/22/2018 12:32 PM, Jan Beulich wrote:
>>>> On 22.05.18 at 18:20, <boris.ostrovsky@oracle.com> wrote:
>> On 05/22/2018 12:10 PM, Jan Beulich wrote:
>>>>>> On 22.05.18 at 17:15, <brgerst@gmail.com> wrote:
>>>> On Tue, May 22, 2018 at 9:57 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 22.05.18 at 15:45, <brgerst@gmail.com> wrote:
>>>>>> On Mon, May 21, 2018 at 11:54 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com> 
>> 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

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

[Jan Beulich]

>>> On 22.05.18 at 19:10, <boris.ostrovsky@oracle.com> wrote:
> On 05/22/2018 12:32 PM, Jan Beulich wrote:
>>>>> On 22.05.18 at 18:20, <boris.ostrovsky@oracle.com> 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

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

[Juergen Gross]

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 <boris.ostrovsky@oracle.com>

With the clearing of EDX (instead using CDQ) you can add my

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[PATCH v4 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 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

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

[Juergen Gross]

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 <boris.ostrovsky@oracle.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen