[PATCH 0/4] PVH GDT fixes and cleanup

[PATCH 0/4] PVH GDT fixes and cleanup

[Boris Ostrovsky]

Switching to new binutils release triggered the first bug.

Not sure if stack canary bug is related to the new tools as well
(haven't checked it with old tools, but they are really old, from
Fedora 13 days).

64-bit guests run fine even without adding the entry for GS but my
guess is that's because Xen toolstack sets cached portions of the
register to sane values and HW makes fewer checks in long mode.
Since those values are not part of the ABI I figured I should fix
it for both 32- and 64-bit mode.


Boris Ostrovsky (4):
  xen/PVH: Replace GDT_ENTRY with explicit constant
  xen/PVH: Use proper CS selector in long mode
  xen/PVH: Set up GS segment for stack canary
  xen/PVH: Remove reserved entry in PVH GDT

 arch/x86/xen/xen-pvh.S | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

-- 
2.9.3

[PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Boris Ostrovsky]

Latest binutils release (2.29.1) will no longer allow proper computation
of GDT entries on 32-bits, with warning:

arch/x86/xen/xen-pvh.S: Assembler messages:
arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)

Use explicit value of the entry instead of using GDT_ENTRY() macro.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: stable@vger.kernel.org
---
 arch/x86/xen/xen-pvh.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index e1a5fbe..934f7d4 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -145,11 +145,11 @@ gdt_start:
 	.quad 0x0000000000000000            /* NULL descriptor */
 	.quad 0x0000000000000000            /* reserved */
 #ifdef CONFIG_X86_64
-	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
+	.quad 0x00af9a000000ffff            /* __BOOT_CS */
 #else
-	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
+	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
 #endif
-	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* __KERNEL_DS */
+	.quad 0x00cf92000000ffff            /* __BOOT_DS */
 gdt_end:
 
 	.balign 4
-- 
2.9.3

[PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: stable@vger.kernel.org
---
 arch/x86/xen/xen-pvh.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index 934f7d4..373fef0 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -93,7 +93,7 @@ ENTRY(pvh_start_xen)
 	mov %eax, %cr0
 
 	/* Jump to 64-bit mode. */
-	ljmp $__KERNEL_CS, $_pa(1f)
+	ljmp $__BOOT_CS, $_pa(1f)
 
 	/* 64-bit entry point. */
 	.code64
-- 
2.9.3

[PATCH 3/4] 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>
Cc: stable@vger.kernel.org
---
 arch/x86/xen/xen-pvh.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index 373fef0..4eed586 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
@@ -150,6 +156,7 @@ gdt_start:
 	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
 #endif
 	.quad 0x00cf92000000ffff            /* __BOOT_DS */
+	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
 gdt_end:
 
 	.balign 4
-- 
2.9.3

[PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Boris Ostrovsky]

And without it we can't use _BOOT_XX macros any longer so define new ones.

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 4eed586..30dd067 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 $__BOOT_CS, $_pa(1f)
+	ljmp $PVH_CS_SEL, $_pa(1f)
 
 	/* 64-bit entry point. */
 	.code64
@@ -122,13 +126,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
@@ -137,7 +141,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)
 
@@ -149,13 +153,12 @@ gdt:
 	.word 0
 gdt_start:
 	.quad 0x0000000000000000            /* NULL descriptor */
-	.quad 0x0000000000000000            /* reserved */
 #ifdef CONFIG_X86_64
-	.quad 0x00af9a000000ffff            /* __BOOT_CS */
+	.quad 0x00af9a000000ffff            /* PVH_CS_SEL */
 #else
-	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
+	.quad 0x00cf9a000000ffff            /* PVH_CS_SEL */
 #endif
-	.quad 0x00cf92000000ffff            /* __BOOT_DS */
+	.quad 0x00cf92000000ffff            /* PVH_DS_SEL */
 	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
 gdt_end:
 
-- 
2.9.3

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Roger Pau Monné]

On Mon, Apr 30, 2018 at 12:23:36PM -0400, Boris Ostrovsky wrote:
> Latest binutils release (2.29.1) will no longer allow proper computation
> of GDT entries on 32-bits, with warning:
> 
> arch/x86/xen/xen-pvh.S: Assembler messages:
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> 
> Use explicit value of the entry instead of using GDT_ENTRY() macro.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/xen/xen-pvh.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
> index e1a5fbe..934f7d4 100644
> --- a/arch/x86/xen/xen-pvh.S
> +++ b/arch/x86/xen/xen-pvh.S
> @@ -145,11 +145,11 @@ gdt_start:
>  	.quad 0x0000000000000000            /* NULL descriptor */
>  	.quad 0x0000000000000000            /* reserved */
>  #ifdef CONFIG_X86_64
> -	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
> +	.quad 0x00af9a000000ffff            /* __BOOT_CS */
>  #else
> -	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
> +	.quad 0x00cf9a000000ffff            /* __BOOT_CS */

Maybe it would be cleaner to use something like:

.word 0xffff /* limit */
.word 0      /* base */
.byte 0      /* base */
.byte 0x9a   /* access */
#ifdef CONFIG_X86_64
.byte 0xaf   /* flags plus limit */
#else
.byte 0xcf   /* flags plus limit */
#endif
.byte 0      /* base */

Or try to fix the GDT_ENTRY macro, maybe if you prepend extra 0's to
make the values 64bit that would prevent the warnings?

Or declare the GDT in enlighten_pvh in C and use it here?

Also, IIRC the base/limit values are ignored in long mode.

Thanks, Roger.

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Boris Ostrovsky]

On 04/30/2018 12:57 PM, Roger Pau Monné wrote:
> On Mon, Apr 30, 2018 at 12:23:36PM -0400, Boris Ostrovsky wrote:
>> Latest binutils release (2.29.1) will no longer allow proper computation
>> of GDT entries on 32-bits, with warning:
>>
>> arch/x86/xen/xen-pvh.S: Assembler messages:
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
>>
>> Use explicit value of the entry instead of using GDT_ENTRY() macro.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  arch/x86/xen/xen-pvh.S | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
>> index e1a5fbe..934f7d4 100644
>> --- a/arch/x86/xen/xen-pvh.S
>> +++ b/arch/x86/xen/xen-pvh.S
>> @@ -145,11 +145,11 @@ gdt_start:
>>  	.quad 0x0000000000000000            /* NULL descriptor */
>>  	.quad 0x0000000000000000            /* reserved */
>>  #ifdef CONFIG_X86_64
>> -	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
>> +	.quad 0x00af9a000000ffff            /* __BOOT_CS */
>>  #else
>> -	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
>> +	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
> Maybe it would be cleaner to use something like:

I actually considered all of these and ended up with a raw number
because it seems to be a convention in kernel (and Xen too, apparently) 
to use raw values in .S files.

Kernel is using now GDT_ENTRY_INIT() which is a C macro. There is one
other location where GDT_INIT() is used (arch/x86/boot/pm.c) and,
incidentally, it also generates this warning IIRC.

I really don't want to move definition to C code just to use a macro ---
I don't think C code needs to be exposed to this GDT.


>
> .word 0xffff /* limit */
> .word 0      /* base */
> .byte 0      /* base */
> .byte 0x9a   /* access */
> #ifdef CONFIG_X86_64
> .byte 0xaf   /* flags plus limit */
> #else
> .byte 0xcf   /* flags plus limit */
> #endif
> .byte 0      /* base */


I, in fact, started with something like this. But if you repeat this 4
times you will probably see why I decided against it ;-)


-boris


>
> Or try to fix the GDT_ENTRY macro, maybe if you prepend extra 0's to
> make the values 64bit that would prevent the warnings?
>
> Or declare the GDT in enlighten_pvh in C and use it here?
>
> Also, IIRC the base/limit values are ignored in long mode.
>
> Thanks, Roger.

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Roger Pau Monné]

On Mon, Apr 30, 2018 at 02:07:43PM -0400, Boris Ostrovsky wrote:
> On 04/30/2018 12:57 PM, Roger Pau Monné wrote:
> > On Mon, Apr 30, 2018 at 12:23:36PM -0400, Boris Ostrovsky wrote:
> >> Latest binutils release (2.29.1) will no longer allow proper computation
> >> of GDT entries on 32-bits, with warning:
> >>
> >> arch/x86/xen/xen-pvh.S: Assembler messages:
> >> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> >>
> >> Use explicit value of the entry instead of using GDT_ENTRY() macro.
> >>
> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>  arch/x86/xen/xen-pvh.S | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
> >> index e1a5fbe..934f7d4 100644
> >> --- a/arch/x86/xen/xen-pvh.S
> >> +++ b/arch/x86/xen/xen-pvh.S
> >> @@ -145,11 +145,11 @@ gdt_start:
> >>  	.quad 0x0000000000000000            /* NULL descriptor */
> >>  	.quad 0x0000000000000000            /* reserved */
> >>  #ifdef CONFIG_X86_64
> >> -	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
> >> +	.quad 0x00af9a000000ffff            /* __BOOT_CS */
> >>  #else
> >> -	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
> >> +	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
> > Maybe it would be cleaner to use something like:
> 
> I actually considered all of these and ended up with a raw number
> because it seems to be a convention in kernel (and Xen too, apparently) 
> to use raw values in .S files.
> 
> Kernel is using now GDT_ENTRY_INIT() which is a C macro. There is one
> other location where GDT_INIT() is used (arch/x86/boot/pm.c) and,
> incidentally, it also generates this warning IIRC.
> 
> I really don't want to move definition to C code just to use a macro ---
> I don't think C code needs to be exposed to this GDT.
> 
> 
> >
> > .word 0xffff /* limit */
> > .word 0      /* base */
> > .byte 0      /* base */
> > .byte 0x9a   /* access */
> > #ifdef CONFIG_X86_64
> > .byte 0xaf   /* flags plus limit */
> > #else
> > .byte 0xcf   /* flags plus limit */
> > #endif
> > .byte 0      /* base */
> 
> 
> I, in fact, started with something like this. But if you repeat this 4
> times you will probably see why I decided against it ;-)

Heh, right. Maybe a .macro to generate those? Or this is all too much
for just a couple of GDT entries anyway...

For long mode however you could use simpler values, AFAICT the code
segment in long mode could be simplified to:

0x00209a0000000000

Because the base/limit have no effect.

In any case I'm not specially inclined either way, and maybe using
similar values for 32 and 64bit modes makes this easier to understand
(and decode if needed).

Roger.

Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Roger Pau Monné]

On Mon, Apr 30, 2018 at 12:23:39PM -0400, Boris Ostrovsky wrote:
> And without it we can't use _BOOT_XX macros any longer so define new ones.

Not being that familiar with Linux internals I'm not sure I see the
benefit of this. Isn't there a risk that some other code is going to
use the __BOOT_XX defines?

Thanks, Roger.

RE: [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[David Laight]

From: Boris Ostrovsky
> Sent: 30 April 2018 17:24
> To: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org
> Cc: jgross@suse.com; Boris Ostrovsky; stable@vger.kernel.org
> Subject: [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
> 
> Latest binutils release (2.29.1) will no longer allow proper computation
> of GDT entries on 32-bits, with warning:
> 
> arch/x86/xen/xen-pvh.S: Assembler messages:
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> 
> Use explicit value of the entry instead of using GDT_ENTRY() macro.
...
>  #ifdef CONFIG_X86_64
> -	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
> +	.quad 0x00af9a000000ffff            /* __BOOT_CS */
>  #else
> -	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
> +	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>  #endif
> -	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* __KERNEL_DS */
> +	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>  gdt_end:

It has to be possible to fix the GDT_ENTRY() macro.
Even if you end up with one that generates two 32bit values.

You've also changed the name in the comments.

	David

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Boris Ostrovsky]

On 05/01/2018 03:53 AM, Roger Pau Monné wrote:
> On Mon, Apr 30, 2018 at 02:07:43PM -0400, Boris Ostrovsky wrote:
>> On 04/30/2018 12:57 PM, Roger Pau Monné wrote:
>>> On Mon, Apr 30, 2018 at 12:23:36PM -0400, Boris Ostrovsky wrote:
>>>> Latest binutils release (2.29.1) will no longer allow proper computation
>>>> of GDT entries on 32-bits, with warning:
>>>>
>>>> arch/x86/xen/xen-pvh.S: Assembler messages:
>>>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>>>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
>>>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>>>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
>>>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
>>>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
>>>>
>>>> Use explicit value of the entry instead of using GDT_ENTRY() macro.
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>   arch/x86/xen/xen-pvh.S | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
>>>> index e1a5fbe..934f7d4 100644
>>>> --- a/arch/x86/xen/xen-pvh.S
>>>> +++ b/arch/x86/xen/xen-pvh.S
>>>> @@ -145,11 +145,11 @@ gdt_start:
>>>>   	.quad 0x0000000000000000            /* NULL descriptor */
>>>>   	.quad 0x0000000000000000            /* reserved */
>>>>   #ifdef CONFIG_X86_64
>>>> -	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
>>>> +	.quad 0x00af9a000000ffff            /* __BOOT_CS */
>>>>   #else
>>>> -	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
>>>> +	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>> Maybe it would be cleaner to use something like:
>>
>> I actually considered all of these and ended up with a raw number
>> because it seems to be a convention in kernel (and Xen too, apparently)
>> to use raw values in .S files.
>>
>> Kernel is using now GDT_ENTRY_INIT() which is a C macro. There is one
>> other location where GDT_INIT() is used (arch/x86/boot/pm.c) and,
>> incidentally, it also generates this warning IIRC.
>>
>> I really don't want to move definition to C code just to use a macro ---
>> I don't think C code needs to be exposed to this GDT.
>>
>>
>>>
>>> .word 0xffff /* limit */
>>> .word 0      /* base */
>>> .byte 0      /* base */
>>> .byte 0x9a   /* access */
>>> #ifdef CONFIG_X86_64
>>> .byte 0xaf   /* flags plus limit */
>>> #else
>>> .byte 0xcf   /* flags plus limit */
>>> #endif
>>> .byte 0      /* base */
>>
>>
>> I, in fact, started with something like this. But if you repeat this 4
>> times you will probably see why I decided against it ;-)
> 
> Heh, right. Maybe a .macro to generate those? Or this is all too much
> for just a couple of GDT entries anyway...


That's what I thought. Especially given that assembly code seems to be 
using raw values.

> 
> For long mode however you could use simpler values, AFAICT the code
> segment in long mode could be simplified to:
> 
> 0x00209a0000000000
> 
> Because the base/limit have no effect.


True. However, we are sharing the DS (and later GS) descriptors between 
32- and 64-it modes. I can separate them if you think it makes sense.

-boris


> 
> In any case I'm not specially inclined either way, and maybe using
> similar values for 32 and 64bit modes makes this easier to understand
> (and decode if needed).
> 
> Roger.
> 

Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Boris Ostrovsky]

On 05/01/2018 04:00 AM, Roger Pau Monné wrote:
> On Mon, Apr 30, 2018 at 12:23:39PM -0400, Boris Ostrovsky wrote:
>> And without it we can't use _BOOT_XX macros any longer so define new ones.
> 
> Not being that familiar with Linux internals I'm not sure I see the
> benefit of this. Isn't there a risk that some other code is going to
> use the __BOOT_XX defines?

The startup code we are jumping to loads their own GDT and I don't see 
any explicit references to segments.

The reason I added this patch was that since we are adding another 
segment descriptor (GS) we are now using PVH-specific GDT and so we are 
not sharing layout with other code anymore. (Also, the new GS segment 
overlaps with __BOOT_TSS so I kind of broke it already there, not 
unintentionally).

But if people think I should stick with __BOOT_XX I can drop this patch 
(and then probably move GS down one entry).


-boris

Re: [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Boris Ostrovsky]

On 05/01/2018 07:31 AM, David Laight wrote:
> From: Boris Ostrovsky
>> Sent: 30 April 2018 17:24
>> To: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org
>> Cc: jgross@suse.com; Boris Ostrovsky; stable@vger.kernel.org
>> Subject: [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
>>
>> Latest binutils release (2.29.1) will no longer allow proper computation
>> of GDT entries on 32-bits, with warning:
>>
>> arch/x86/xen/xen-pvh.S: Assembler messages:
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
>>
>> Use explicit value of the entry instead of using GDT_ENTRY() macro.
> ...
>>   #ifdef CONFIG_X86_64
>> -	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
>> +	.quad 0x00af9a000000ffff            /* __BOOT_CS */
>>   #else
>> -	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
>> +	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>   #endif
>> -	.quad GDT_ENTRY(0xc092, 0, 0xfffff) /* __KERNEL_DS */
>> +	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>>   gdt_end:
> 
> It has to be possible to fix the GDT_ENTRY() macro.
> Even if you end up with one that generates two 32bit values.


Is it worth it though? We seem to be using GDT_ENTRY_INIT() everywhere 
and the only other reference that I see is in pm.c and it also probably 
ought to use GDT_ENTRY_INIT().


> 
> You've also changed the name in the comments.

Yes, I should mention this in the commit message.


-boris

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Jan Beulich]

>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
> Latest binutils release (2.29.1) will no longer allow proper computation
> of GDT entries on 32-bits, with warning:
> 
> arch/x86/xen/xen-pvh.S: Assembler messages:
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)

I think this is a mis-configured binutils build - even if targeting 32-bit only, it
should allow 64-bit arithmetic (i.e. be configured with --enable-64-bit-bfd).
Note how, for example, this

	.long	1 << 32, 0x10000 * 0x10000
	.quad	1 << 32, 0x10000 * 0x10000

assembles consistently with a warning on _both_ values on the first line
with what I'd call a properly configured binutils build, but errors only on
the shift expressions on each line for an (imo) improperly configured one.
The only viable alternative would imo be to simply disallow .quad without
--enable-64-bit-bfd, but I guess that would break a number of consumers.

In any event I'd like to suggest to drop this patch.

Jan

Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Jan Beulich]

>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

But to understand why things have been working nevertheless it would
have been nice if the commit message wasn't empty, but instead said
something like "The two happen to be identical on 64-bit."

Jan

Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Jan Beulich]

>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
> --- 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)

I can only advise against doing it this way: There's no safeguard against
someone changing asm/segment.h without changing this value (in fact
this applies to all of the GDT selectors populated in this file). At the very
least tie this to GDT_ENTRY_BOOT_TSS / __BOOT_TSS?

> @@ -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
> @@ -150,6 +156,7 @@ gdt_start:
>  	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>  #endif
>  	.quad 0x00cf92000000ffff            /* __BOOT_DS */
> +	.quad 0x0040900000000018            /* PVH_CANARY_SEL */

Without any further code before loading the selector, this points at
physical address 0. Don't you need to add in the base address of
the per-CPU stack_canary?

Jan

Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Jan Beulich]

>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
> And without it we can't use _BOOT_XX macros any longer so define new ones.

Ah, here we go. Perhaps this should be moved earlier in the series?
Assuming you really want to go this route in the first place, taking
Roger's comment into consideration.

Jan

Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Jan Beulich]

>>> On 01.05.18 at 14:34, <boris.ostrovsky@oracle.com> wrote:
> On 05/01/2018 04:00 AM, Roger Pau Monné wrote:
>> On Mon, Apr 30, 2018 at 12:23:39PM -0400, Boris Ostrovsky wrote:
>>> And without it we can't use _BOOT_XX macros any longer so define new ones.
>> 
>> Not being that familiar with Linux internals I'm not sure I see the
>> benefit of this. Isn't there a risk that some other code is going to
>> use the __BOOT_XX defines?
> 
> The startup code we are jumping to loads their own GDT and I don't see 
> any explicit references to segments.

No explicit references to segments isn't enough: You also need to make
sure no exceptions at all can occur while loaded selectors and GDT are
out of sync - in particular NMI might be of concern here (this isn't PV
after all, where not having a callback registered effectively masks NMI).

Jan

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Boris Ostrovsky]

On 05/02/2018 04:00 AM, Jan Beulich wrote:
>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>> Latest binutils release (2.29.1) will no longer allow proper computation
>> of GDT entries on 32-bits, with warning:
>>
>> arch/x86/xen/xen-pvh.S: Assembler messages:
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> I think this is a mis-configured binutils build - even if targeting 32-bit only, it
> should allow 64-bit arithmetic (i.e. be configured with --enable-64-bit-bfd).
> Note how, for example, this
>
> 	.long	1 << 32, 0x10000 * 0x10000
> 	.quad	1 << 32, 0x10000 * 0x10000
>
> assembles consistently with a warning on _both_ values on the first line
> with what I'd call a properly configured binutils build, but errors only on
> the shift expressions on each line for an (imo) improperly configured one.
> The only viable alternative would imo be to simply disallow .quad without
> --enable-64-bit-bfd, but I guess that would break a number of consumers.
>
> In any event I'd like to suggest to drop this patch.


Let me see go back and see how I built my binutils. And maybe rebuild
them on something > fedora13.

-boris

Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Boris Ostrovsky]

On 05/02/2018 04:05 AM, Jan Beulich wrote:
>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> But to understand why things have been working nevertheless it would
> have been nice if the commit message wasn't empty, but instead said
> something like "The two happen to be identical on 64-bit."


Why do you think they are identical? __KERNEL_CS points to entry#12
(which we don't specify in PVH GDT) while __BOOT_CS is the second entry
(which we do create).

-boris

Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Jan Beulich]

>>> On 02.05.18 at 16:57, <boris.ostrovsky@oracle.com> wrote:
> On 05/02/2018 04:05 AM, Jan Beulich wrote:
>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> But to understand why things have been working nevertheless it would
>> have been nice if the commit message wasn't empty, but instead said
>> something like "The two happen to be identical on 64-bit."
> 
> 
> Why do you think they are identical? __KERNEL_CS points to entry#12
> (which we don't specify in PVH GDT) while __BOOT_CS is the second entry
> (which we do create).

That's 32-bit's __KERNEL_CS. If the two weren't identical, the ljmp
you adjust would never have worked afaict.

Jan

Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Boris Ostrovsky]

On 05/02/2018 04:16 AM, Jan Beulich wrote:
>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>> --- 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)
> I can only advise against doing it this way: There's no safeguard against
> someone changing asm/segment.h without changing this value (in fact
> this applies to all of the GDT selectors populated in this file). At the very
> least tie this to GDT_ENTRY_BOOT_TSS / __BOOT_TSS?
>
>> @@ -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
>> @@ -150,6 +156,7 @@ gdt_start:
>>  	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>  #endif
>>  	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>> +	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
> Without any further code before loading the selector, this points at
> physical address 0. Don't you need to add in the base address of
> the per-CPU stack_canary?

This GDT is gone soon after we jump into generic x86 startup code.That
code will load its own GDT (and then set up per-cpu segments and all that).

-boris

Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Jan Beulich]

>>> On 02.05.18 at 17:00, <boris.ostrovsky@oracle.com> wrote:
> On 05/02/2018 04:16 AM, Jan Beulich wrote:
>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>> --- 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)
>> I can only advise against doing it this way: There's no safeguard against
>> someone changing asm/segment.h without changing this value (in fact
>> this applies to all of the GDT selectors populated in this file). At the 
> very
>> least tie this to GDT_ENTRY_BOOT_TSS / __BOOT_TSS?
>>
>>> @@ -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
>>> @@ -150,6 +156,7 @@ gdt_start:
>>>  	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>>  #endif
>>>  	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>>> +	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
>> Without any further code before loading the selector, this points at
>> physical address 0. Don't you need to add in the base address of
>> the per-CPU stack_canary?
> 
> This GDT is gone soon after we jump into generic x86 startup code.That
> code will load its own GDT (and then set up per-cpu segments and all that).

All understood, but why would you set up the per-CPU segment here if
what you load into the segment register is not usable for the intended
purpose (until that other code sets up things and reloads the segment
registers)?

Jan

Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Boris Ostrovsky]

On 05/02/2018 04:26 AM, Jan Beulich wrote:
>>>> On 01.05.18 at 14:34, <boris.ostrovsky@oracle.com> wrote:
>> On 05/01/2018 04:00 AM, Roger Pau Monné wrote:
>>> On Mon, Apr 30, 2018 at 12:23:39PM -0400, Boris Ostrovsky wrote:
>>>> And without it we can't use _BOOT_XX macros any longer so define new ones.
>>> Not being that familiar with Linux internals I'm not sure I see the
>>> benefit of this. Isn't there a risk that some other code is going to
>>> use the __BOOT_XX defines?
>> The startup code we are jumping to loads their own GDT and I don't see 
>> any explicit references to segments.
> No explicit references to segments isn't enough: You also need to make
> sure no exceptions at all can occur while loaded selectors and GDT are
> out of sync - in particular NMI might be of concern here (this isn't PV
> after all, where not having a callback registered effectively masks NMI).

How would keeping __BOOT_XX selectors help with NMI? We don't have
anything set up for NMI handling anyway yet, this is all done in x86
startup code later.

-boris

Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Jan Beulich]

>>> On 02.05.18 at 17:06, <boris.ostrovsky@oracle.com> wrote:
> On 05/02/2018 04:26 AM, Jan Beulich wrote:
>>>>> On 01.05.18 at 14:34, <boris.ostrovsky@oracle.com> wrote:
>>> On 05/01/2018 04:00 AM, Roger Pau Monné wrote:
>>>> On Mon, Apr 30, 2018 at 12:23:39PM -0400, Boris Ostrovsky wrote:
>>>>> And without it we can't use _BOOT_XX macros any longer so define new ones.
>>>> Not being that familiar with Linux internals I'm not sure I see the
>>>> benefit of this. Isn't there a risk that some other code is going to
>>>> use the __BOOT_XX defines?
>>> The startup code we are jumping to loads their own GDT and I don't see 
>>> any explicit references to segments.
>> No explicit references to segments isn't enough: You also need to make
>> sure no exceptions at all can occur while loaded selectors and GDT are
>> out of sync - in particular NMI might be of concern here (this isn't PV
>> after all, where not having a callback registered effectively masks NMI).
> 
> How would keeping __BOOT_XX selectors help with NMI? We don't have
> anything set up for NMI handling anyway yet, this is all done in x86
> startup code later.

Oh, you're right - there's no IDT either, so an NMI would yield a triple fault
anyway.

Jan

Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Boris Ostrovsky]

On 05/02/2018 11:00 AM, Jan Beulich wrote:
>>>> On 02.05.18 at 16:57, <boris.ostrovsky@oracle.com> wrote:
>> On 05/02/2018 04:05 AM, Jan Beulich wrote:
>>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> But to understand why things have been working nevertheless it would
>>> have been nice if the commit message wasn't empty, but instead said
>>> something like "The two happen to be identical on 64-bit."
>>
>> Why do you think they are identical? __KERNEL_CS points to entry#12
>> (which we don't specify in PVH GDT) while __BOOT_CS is the second entry
>> (which we do create).
> That's 32-bit's __KERNEL_CS. If the two weren't identical, the ljmp
> you adjust would never have worked afaict.


Oh, right. My theory was that we were picking up something from the
stack (which is where 12th entry would be pointing) and the L bit, which
I think is the only one we'd care about, happened to always be set there.

-boris

Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Jan Beulich]

>>> On 02.05.18 at 17:08, <boris.ostrovsky@oracle.com> wrote:
> On 05/02/2018 11:00 AM, Jan Beulich wrote:
>>>>> On 02.05.18 at 16:57, <boris.ostrovsky@oracle.com> wrote:
>>> On 05/02/2018 04:05 AM, Jan Beulich wrote:
>>>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> But to understand why things have been working nevertheless it would
>>>> have been nice if the commit message wasn't empty, but instead said
>>>> something like "The two happen to be identical on 64-bit."
>>>
>>> Why do you think they are identical? __KERNEL_CS points to entry#12
>>> (which we don't specify in PVH GDT) while __BOOT_CS is the second entry
>>> (which we do create).
>> That's 32-bit's __KERNEL_CS. If the two weren't identical, the ljmp
>> you adjust would never have worked afaict.
> 
> 
> Oh, right. My theory was that we were picking up something from the
> stack (which is where 12th entry would be pointing) and the L bit, which
> I think is the only one we'd care about, happened to always be set there.

I don't think the L bit is the only one we care about, as I don't think you
can load a non-code selector into CS (even if none of the attributes are
later used for anything).

Jan

Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Boris Ostrovsky]

On 05/02/2018 11:01 AM, Jan Beulich wrote:
>>>> On 02.05.18 at 17:00, <boris.ostrovsky@oracle.com> wrote:
>> On 05/02/2018 04:16 AM, Jan Beulich wrote:
>>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>>> --- 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)
>>> I can only advise against doing it this way: There's no safeguard against
>>> someone changing asm/segment.h without changing this value (in fact
>>> this applies to all of the GDT selectors populated in this file). At the 
>> very
>>> least tie this to GDT_ENTRY_BOOT_TSS / __BOOT_TSS?
>>>
>>>> @@ -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
>>>> @@ -150,6 +156,7 @@ gdt_start:
>>>>  	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>>>  #endif
>>>>  	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>>>> +	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
>>> Without any further code before loading the selector, this points at
>>> physical address 0. Don't you need to add in the base address of
>>> the per-CPU stack_canary?
>> This GDT is gone soon after we jump into generic x86 startup code.That
>> code will load its own GDT (and then set up per-cpu segments and all that).
> All understood, but why would you set up the per-CPU segment here if
> what you load into the segment register is not usable for the intended
> purpose (until that other code sets up things and reloads the segment
> registers)?

The intended purpose here is to allow stack protector access not to
fail. At this point it doesn't really matter that GS is later used for
per-cpu segment, this code (and this GDT) will not be used when other
CPUs come up.

-boris

Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Andrew Cooper]

On 02/05/18 16:09, Jan Beulich wrote:
>>>> On 02.05.18 at 17:08, <boris.ostrovsky@oracle.com> wrote:
>> On 05/02/2018 11:00 AM, Jan Beulich wrote:
>>>>>> On 02.05.18 at 16:57, <boris.ostrovsky@oracle.com> wrote:
>>>> On 05/02/2018 04:05 AM, Jan Beulich wrote:
>>>>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> But to understand why things have been working nevertheless it would
>>>>> have been nice if the commit message wasn't empty, but instead said
>>>>> something like "The two happen to be identical on 64-bit."
>>>> Why do you think they are identical? __KERNEL_CS points to entry#12
>>>> (which we don't specify in PVH GDT) while __BOOT_CS is the second entry
>>>> (which we do create).
>>> That's 32-bit's __KERNEL_CS. If the two weren't identical, the ljmp
>>> you adjust would never have worked afaict.
>>
>> Oh, right. My theory was that we were picking up something from the
>> stack (which is where 12th entry would be pointing) and the L bit, which
>> I think is the only one we'd care about, happened to always be set there.
> I don't think the L bit is the only one we care about, as I don't think you
> can load a non-code selector into CS (even if none of the attributes are
> later used for anything).

The type/s/dpl/p/d/l attributes still very much matter even in 64bit.

~Andrew

Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Jan Beulich]

>>> On 02.05.18 at 17:22, <boris.ostrovsky@oracle.com> wrote:
> On 05/02/2018 11:01 AM, Jan Beulich wrote:
>>>>> On 02.05.18 at 17:00, <boris.ostrovsky@oracle.com> wrote:
>>> On 05/02/2018 04:16 AM, Jan Beulich wrote:
>>>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>>>> --- 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)
>>>> I can only advise against doing it this way: There's no safeguard against
>>>> someone changing asm/segment.h without changing this value (in fact
>>>> this applies to all of the GDT selectors populated in this file). At the 
>>> very
>>>> least tie this to GDT_ENTRY_BOOT_TSS / __BOOT_TSS?
>>>>
>>>>> @@ -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
>>>>> @@ -150,6 +156,7 @@ gdt_start:
>>>>>  	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>>>>  #endif
>>>>>  	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>>>>> +	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
>>>> Without any further code before loading the selector, this points at
>>>> physical address 0. Don't you need to add in the base address of
>>>> the per-CPU stack_canary?
>>> This GDT is gone soon after we jump into generic x86 startup code.That
>>> code will load its own GDT (and then set up per-cpu segments and all that).
>> All understood, but why would you set up the per-CPU segment here if
>> what you load into the segment register is not usable for the intended
>> purpose (until that other code sets up things and reloads the segment
>> registers)?
> 
> The intended purpose here is to allow stack protector access not to
> fail. At this point it doesn't really matter that GS is later used for
> per-cpu segment, this code (and this GDT) will not be used when other
> CPUs come up.

But the place the canary would live this way is completely wrong. Anyway,
you're the maintainer of this code, so I guess I better shut up now.

Jan

Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Boris Ostrovsky]

On 05/02/2018 11:41 AM, Jan Beulich wrote:
>>>> On 02.05.18 at 17:22, <boris.ostrovsky@oracle.com> wrote:
>> On 05/02/2018 11:01 AM, Jan Beulich wrote:
>>>>>> On 02.05.18 at 17:00, <boris.ostrovsky@oracle.com> wrote:
>>>> On 05/02/2018 04:16 AM, Jan Beulich wrote:
>>>>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>>>>> --- 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)
>>>>> I can only advise against doing it this way: There's no safeguard against
>>>>> someone changing asm/segment.h without changing this value (in fact
>>>>> this applies to all of the GDT selectors populated in this file). At the 
>>>> very
>>>>> least tie this to GDT_ENTRY_BOOT_TSS / __BOOT_TSS?
>>>>>
>>>>>> @@ -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
>>>>>> @@ -150,6 +156,7 @@ gdt_start:
>>>>>>  	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>>>>>  #endif
>>>>>>  	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>>>>>> +	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
>>>>> Without any further code before loading the selector, this points at
>>>>> physical address 0. Don't you need to add in the base address of
>>>>> the per-CPU stack_canary?
>>>> This GDT is gone soon after we jump into generic x86 startup code.That
>>>> code will load its own GDT (and then set up per-cpu segments and all that).
>>> All understood, but why would you set up the per-CPU segment here if
>>> what you load into the segment register is not usable for the intended
>>> purpose (until that other code sets up things and reloads the segment
>>> registers)?
>> The intended purpose here is to allow stack protector access not to
>> fail. At this point it doesn't really matter that GS is later used for
>> per-cpu segment, this code (and this GDT) will not be used when other
>> CPUs come up.
> But the place the canary would live this way is completely wrong. 


Would creating a canary variable and using it as a base address be better?


-boris

Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Jan Beulich]

>>> On 02.05.18 at 19:29, <boris.ostrovsky@oracle.com> wrote:
> On 05/02/2018 11:41 AM, Jan Beulich wrote:
>>>>> On 02.05.18 at 17:22, <boris.ostrovsky@oracle.com> wrote:
>>> On 05/02/2018 11:01 AM, Jan Beulich wrote:
>>>>>>> On 02.05.18 at 17:00, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 05/02/2018 04:16 AM, Jan Beulich wrote:
>>>>>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> --- 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)
>>>>>> I can only advise against doing it this way: There's no safeguard against
>>>>>> someone changing asm/segment.h without changing this value (in fact
>>>>>> this applies to all of the GDT selectors populated in this file). At the 
>>>>> very
>>>>>> least tie this to GDT_ENTRY_BOOT_TSS / __BOOT_TSS?
>>>>>>
>>>>>>> @@ -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
>>>>>>> @@ -150,6 +156,7 @@ gdt_start:
>>>>>>>  	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
>>>>>>>  #endif
>>>>>>>  	.quad 0x00cf92000000ffff            /* __BOOT_DS */
>>>>>>> +	.quad 0x0040900000000018            /* PVH_CANARY_SEL */
>>>>>> Without any further code before loading the selector, this points at
>>>>>> physical address 0. Don't you need to add in the base address of
>>>>>> the per-CPU stack_canary?
>>>>> This GDT is gone soon after we jump into generic x86 startup code.That
>>>>> code will load its own GDT (and then set up per-cpu segments and all that).
>>>> All understood, but why would you set up the per-CPU segment here if
>>>> what you load into the segment register is not usable for the intended
>>>> purpose (until that other code sets up things and reloads the segment
>>>> registers)?
>>> The intended purpose here is to allow stack protector access not to
>>> fail. At this point it doesn't really matter that GS is later used for
>>> per-cpu segment, this code (and this GDT) will not be used when other
>>> CPUs come up.
>> But the place the canary would live this way is completely wrong. 
> 
> 
> Would creating a canary variable and using it as a base address be better?

Of course, because then at least you properly control where an eventual
access would go, instead of touching some unrelated memory location.

Jan