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