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@xxxxxxxxxx> wrote:
> On 05/02/2018 04:16 AM, Jan Beulich wrote:
>>>>> On 30.04.18 at 18:23, <boris.ostrovsky@xxxxxxxxxx> 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