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
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
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
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Boris Ostrovsky -
Jan Beulich