LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/4] PVH GDT fixes and cleanup
@ 2018-04-30 16:23 Boris Ostrovsky
  2018-04-30 16:23 ` [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant Boris Ostrovsky
                   ` (3 more replies)
  0 siblings, 4 replies; 32+ messages in thread
From: Boris Ostrovsky @ 2018-04-30 16:23 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: jgross, 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-04-30 16:23 [PATCH 0/4] PVH GDT fixes and cleanup Boris Ostrovsky
@ 2018-04-30 16:23 ` Boris Ostrovsky
  2018-04-30 16:57   ` [Xen-devel] " Roger Pau Monné
                     ` (2 more replies)
  2018-04-30 16:23 ` [PATCH 2/4] xen/PVH: Use proper CS selector in long mode Boris Ostrovsky
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 32+ messages in thread
From: Boris Ostrovsky @ 2018-04-30 16:23 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: jgross, Boris Ostrovsky, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 2/4] xen/PVH: Use proper CS selector in long mode
  2018-04-30 16:23 [PATCH 0/4] PVH GDT fixes and cleanup Boris Ostrovsky
  2018-04-30 16:23 ` [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant Boris Ostrovsky
@ 2018-04-30 16:23 ` Boris Ostrovsky
  2018-05-02  8:05   ` [Xen-devel] " Jan Beulich
  2018-04-30 16:23 ` [PATCH 3/4] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
  2018-04-30 16:23 ` [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT Boris Ostrovsky
  3 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-04-30 16:23 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: jgross, Boris Ostrovsky, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-04-30 16:23 [PATCH 0/4] PVH GDT fixes and cleanup Boris Ostrovsky
  2018-04-30 16:23 ` [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant Boris Ostrovsky
  2018-04-30 16:23 ` [PATCH 2/4] xen/PVH: Use proper CS selector in long mode Boris Ostrovsky
@ 2018-04-30 16:23 ` Boris Ostrovsky
  2018-05-02  8:16   ` [Xen-devel] " Jan Beulich
  2018-04-30 16:23 ` [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT Boris Ostrovsky
  3 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-04-30 16:23 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: jgross, Boris Ostrovsky, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT
  2018-04-30 16:23 [PATCH 0/4] PVH GDT fixes and cleanup Boris Ostrovsky
                   ` (2 preceding siblings ...)
  2018-04-30 16:23 ` [PATCH 3/4] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
@ 2018-04-30 16:23 ` Boris Ostrovsky
  2018-05-01  8:00   ` [Xen-devel] " Roger Pau Monné
  2018-05-02  8:19   ` Jan Beulich
  3 siblings, 2 replies; 32+ messages in thread
From: Boris Ostrovsky @ 2018-04-30 16:23 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: jgross, 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-04-30 16:23 ` [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant Boris Ostrovsky
@ 2018-04-30 16:57   ` Roger Pau Monné
  2018-04-30 18:07     ` Boris Ostrovsky
  2018-05-01 11:31   ` David Laight
  2018-05-02  8:00   ` [Xen-devel] " Jan Beulich
  2 siblings, 1 reply; 32+ messages in thread
From: Roger Pau Monné @ 2018-04-30 16:57 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: linux-kernel, xen-devel, jgross, stable

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.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-04-30 16:57   ` [Xen-devel] " Roger Pau Monné
@ 2018-04-30 18:07     ` Boris Ostrovsky
  2018-05-01  7:53       ` Roger Pau Monné
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-04-30 18:07 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: linux-kernel, xen-devel, jgross, stable

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.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-04-30 18:07     ` Boris Ostrovsky
@ 2018-05-01  7:53       ` Roger Pau Monné
  2018-05-01 12:16         ` Boris Ostrovsky
  0 siblings, 1 reply; 32+ messages in thread
From: Roger Pau Monné @ 2018-05-01  7:53 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: linux-kernel, xen-devel, jgross, stable

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.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT
  2018-04-30 16:23 ` [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT Boris Ostrovsky
@ 2018-05-01  8:00   ` Roger Pau Monné
  2018-05-01 12:34     ` Boris Ostrovsky
  2018-05-02  8:19   ` Jan Beulich
  1 sibling, 1 reply; 32+ messages in thread
From: Roger Pau Monné @ 2018-05-01  8:00 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: linux-kernel, xen-devel, jgross

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.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-04-30 16:23 ` [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant Boris Ostrovsky
  2018-04-30 16:57   ` [Xen-devel] " Roger Pau Monné
@ 2018-05-01 11:31   ` David Laight
  2018-05-01 12:40     ` Boris Ostrovsky
  2018-05-02  8:00   ` [Xen-devel] " Jan Beulich
  2 siblings, 1 reply; 32+ messages in thread
From: David Laight @ 2018-05-01 11:31 UTC (permalink / raw)
  To: 'Boris Ostrovsky', linux-kernel, xen-devel; +Cc: jgross, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-05-01  7:53       ` Roger Pau Monné
@ 2018-05-01 12:16         ` Boris Ostrovsky
  0 siblings, 0 replies; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-01 12:16 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: linux-kernel, xen-devel, jgross, stable



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.
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT
  2018-05-01  8:00   ` [Xen-devel] " Roger Pau Monné
@ 2018-05-01 12:34     ` Boris Ostrovsky
  2018-05-02  8:26       ` Jan Beulich
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-01 12:34 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: linux-kernel, xen-devel, jgross



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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-05-01 11:31   ` David Laight
@ 2018-05-01 12:40     ` Boris Ostrovsky
  0 siblings, 0 replies; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-01 12:40 UTC (permalink / raw)
  To: David Laight, linux-kernel, xen-devel; +Cc: jgross, stable



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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-04-30 16:23 ` [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant Boris Ostrovsky
  2018-04-30 16:57   ` [Xen-devel] " Roger Pau Monné
  2018-05-01 11:31   ` David Laight
@ 2018-05-02  8:00   ` Jan Beulich
  2018-05-02 14:53     ` Boris Ostrovsky
  2 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02  8:00 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode
  2018-04-30 16:23 ` [PATCH 2/4] xen/PVH: Use proper CS selector in long mode Boris Ostrovsky
@ 2018-05-02  8:05   ` Jan Beulich
  2018-05-02 14:57     ` Boris Ostrovsky
  0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02  8:05 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-04-30 16:23 ` [PATCH 3/4] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
@ 2018-05-02  8:16   ` Jan Beulich
  2018-05-02 15:00     ` Boris Ostrovsky
  0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02  8:16 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT
  2018-04-30 16:23 ` [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT Boris Ostrovsky
  2018-05-01  8:00   ` [Xen-devel] " Roger Pau Monné
@ 2018-05-02  8:19   ` Jan Beulich
  1 sibling, 0 replies; 32+ messages in thread
From: Jan Beulich @ 2018-05-02  8:19 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT
  2018-05-01 12:34     ` Boris Ostrovsky
@ 2018-05-02  8:26       ` Jan Beulich
  2018-05-02 15:06         ` Boris Ostrovsky
  0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02  8:26 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: Roger Pau Monne, xen-devel, Juergen Gross, linux-kernel

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant
  2018-05-02  8:00   ` [Xen-devel] " Jan Beulich
@ 2018-05-02 14:53     ` Boris Ostrovsky
  0 siblings, 0 replies; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-02 14:53 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode
  2018-05-02  8:05   ` [Xen-devel] " Jan Beulich
@ 2018-05-02 14:57     ` Boris Ostrovsky
  2018-05-02 15:00       ` Jan Beulich
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-02 14:57 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode
  2018-05-02 14:57     ` Boris Ostrovsky
@ 2018-05-02 15:00       ` Jan Beulich
  2018-05-02 15:08         ` Boris Ostrovsky
  0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02 15:00 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-05-02  8:16   ` [Xen-devel] " Jan Beulich
@ 2018-05-02 15:00     ` Boris Ostrovsky
  2018-05-02 15:01       ` Jan Beulich
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-02 15:00 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-05-02 15:00     ` Boris Ostrovsky
@ 2018-05-02 15:01       ` Jan Beulich
  2018-05-02 15:22         ` Boris Ostrovsky
  0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02 15:01 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT
  2018-05-02  8:26       ` Jan Beulich
@ 2018-05-02 15:06         ` Boris Ostrovsky
  2018-05-02 15:07           ` Jan Beulich
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-02 15:06 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Roger Pau Monne, xen-devel, Juergen Gross, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT
  2018-05-02 15:06         ` Boris Ostrovsky
@ 2018-05-02 15:07           ` Jan Beulich
  0 siblings, 0 replies; 32+ messages in thread
From: Jan Beulich @ 2018-05-02 15:07 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: Roger Pau Monne, xen-devel, Juergen Gross, linux-kernel

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode
  2018-05-02 15:00       ` Jan Beulich
@ 2018-05-02 15:08         ` Boris Ostrovsky
  2018-05-02 15:09           ` Jan Beulich
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-02 15:08 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode
  2018-05-02 15:08         ` Boris Ostrovsky
@ 2018-05-02 15:09           ` Jan Beulich
  2018-05-02 15:28             ` Andrew Cooper
  0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02 15:09 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-05-02 15:01       ` Jan Beulich
@ 2018-05-02 15:22         ` Boris Ostrovsky
  2018-05-02 15:41           ` Jan Beulich
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-02 15:22 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode
  2018-05-02 15:09           ` Jan Beulich
@ 2018-05-02 15:28             ` Andrew Cooper
  0 siblings, 0 replies; 32+ messages in thread
From: Andrew Cooper @ 2018-05-02 15:28 UTC (permalink / raw)
  To: Jan Beulich, Boris Ostrovsky
  Cc: Juergen Gross, xen-devel, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-05-02 15:22         ` Boris Ostrovsky
@ 2018-05-02 15:41           ` Jan Beulich
  2018-05-02 17:29             ` Boris Ostrovsky
  0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-05-02 15:41 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-05-02 15:41           ` Jan Beulich
@ 2018-05-02 17:29             ` Boris Ostrovsky
  2018-05-03  7:35               ` Jan Beulich
  0 siblings, 1 reply; 32+ messages in thread
From: Boris Ostrovsky @ 2018-05-02 17:29 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary
  2018-05-02 17:29             ` Boris Ostrovsky
@ 2018-05-03  7:35               ` Jan Beulich
  0 siblings, 0 replies; 32+ messages in thread
From: Jan Beulich @ 2018-05-03  7:35 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: xen-devel, Juergen Gross, linux-kernel, stable

>>> 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2018-05-03  7:35 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-30 16:23 [PATCH 0/4] PVH GDT fixes and cleanup Boris Ostrovsky
2018-04-30 16:23 ` [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant Boris Ostrovsky
2018-04-30 16:57   ` [Xen-devel] " Roger Pau Monné
2018-04-30 18:07     ` Boris Ostrovsky
2018-05-01  7:53       ` Roger Pau Monné
2018-05-01 12:16         ` Boris Ostrovsky
2018-05-01 11:31   ` David Laight
2018-05-01 12:40     ` Boris Ostrovsky
2018-05-02  8:00   ` [Xen-devel] " Jan Beulich
2018-05-02 14:53     ` Boris Ostrovsky
2018-04-30 16:23 ` [PATCH 2/4] xen/PVH: Use proper CS selector in long mode Boris Ostrovsky
2018-05-02  8:05   ` [Xen-devel] " Jan Beulich
2018-05-02 14:57     ` Boris Ostrovsky
2018-05-02 15:00       ` Jan Beulich
2018-05-02 15:08         ` Boris Ostrovsky
2018-05-02 15:09           ` Jan Beulich
2018-05-02 15:28             ` Andrew Cooper
2018-04-30 16:23 ` [PATCH 3/4] xen/PVH: Set up GS segment for stack canary Boris Ostrovsky
2018-05-02  8:16   ` [Xen-devel] " Jan Beulich
2018-05-02 15:00     ` Boris Ostrovsky
2018-05-02 15:01       ` Jan Beulich
2018-05-02 15:22         ` Boris Ostrovsky
2018-05-02 15:41           ` Jan Beulich
2018-05-02 17:29             ` Boris Ostrovsky
2018-05-03  7:35               ` Jan Beulich
2018-04-30 16:23 ` [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT Boris Ostrovsky
2018-05-01  8:00   ` [Xen-devel] " Roger Pau Monné
2018-05-01 12:34     ` Boris Ostrovsky
2018-05-02  8:26       ` Jan Beulich
2018-05-02 15:06         ` Boris Ostrovsky
2018-05-02 15:07           ` Jan Beulich
2018-05-02  8:19   ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).