Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

2018-05-18 Thread Jan Beulich 
>>> On 17.05.18 at 19:47,  wrote:
> On 05/17/2018 11:02 AM, Jan Beulich wrote:
> On 17.05.18 at 16:47,  wrote:
>>> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>>> mov %eax,%es
>>> mov %eax,%ss
>>>  
>>> +   mov $PVH_CANARY_SEL,%eax
>>> +   mov %eax,%gs
>> I doubt this is needed for 64-bit (you could equally well load zero or leave
>> in place what's there in that case),
> 
> I don't understand this.

The actual selector value doesn't matter on 64-bit. Hence you could
omit the load altogether, or you could use plain zero. No need for the
(non-zero) selector, or (by implication) the GDT descriptor.

>>  and loading the selector before setting
>> the base address in the descriptor won't have the intended effect.
> 
> 
> I wasn't sure about this either but then I noticed that
> secondary_startup_64() does it in the same order (although not using the
> MSR).

Well, for one they load a null selector, which is independent of setting up
any GDT descriptors. I also don't understand why you say "although not
using the MSR" when they clearly do. And then, as said above (and also
in a comment in secondary_startup_64()), the actual selector value (and
when / if at all it is loaded) doesn't matter on 64-bit. The ordering does
matter on 32-bit though.

Jan

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

2018-05-18 Thread Jan Beulich 
>>> On 17.05.18 at 19:47,  wrote:
> On 05/17/2018 11:02 AM, Jan Beulich wrote:
> On 17.05.18 at 16:47,  wrote:
>>> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>>> mov %eax,%es
>>> mov %eax,%ss
>>>  
>>> +   mov $PVH_CANARY_SEL,%eax
>>> +   mov %eax,%gs
>> I doubt this is needed for 64-bit (you could equally well load zero or leave
>> in place what's there in that case),
> 
> I don't understand this.

The actual selector value doesn't matter on 64-bit. Hence you could
omit the load altogether, or you could use plain zero. No need for the
(non-zero) selector, or (by implication) the GDT descriptor.

>>  and loading the selector before setting
>> the base address in the descriptor won't have the intended effect.
> 
> 
> I wasn't sure about this either but then I noticed that
> secondary_startup_64() does it in the same order (although not using the
> MSR).

Well, for one they load a null selector, which is independent of setting up
any GDT descriptors. I also don't understand why you say "although not
using the MSR" when they clearly do. And then, as said above (and also
in a comment in secondary_startup_64()), the actual selector value (and
when / if at all it is loaded) doesn't matter on 64-bit. The ordering does
matter on 32-bit though.

Jan

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

2018-05-17 Thread Boris Ostrovsky 
On 05/17/2018 11:02 AM, Jan Beulich wrote:
 On 17.05.18 at 16:47,  wrote:
>> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>>  mov %eax,%es
>>  mov %eax,%ss
>>  
>> +mov $PVH_CANARY_SEL,%eax
>> +mov %eax,%gs
> I doubt this is needed for 64-bit (you could equally well load zero or leave
> in place what's there in that case),

I don't understand this.


>  and loading the selector before setting
> the base address in the descriptor won't have the intended effect.


I wasn't sure about this either but then I noticed that
secondary_startup_64() does it in the same order (although not using the
MSR).


>
>> @@ -150,9 +170,12 @@ gdt_start:
>>  .quad GDT_ENTRY(0xc09a, 0, 0xf) /* __KERNEL_CS */
>>  #endif
>>  .quad GDT_ENTRY(0xc092, 0, 0xf) /* __KERNEL_DS */
>> +.quad GDT_ENTRY(0x4090, 0, 0x18)/* PVH_CANARY_SEL */
>>  gdt_end:
>>  
>> -.balign 4
>> +.balign 16
>> +canary:
>> +.fill 24, 1, 0
> This is too little space for 64-bit afaict (the canary lives at offset 40 
> there
> if I can trust asm/processor.h).

Yes, should be 48. I didn't realize the two modes use different offsets.

-boris

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

2018-05-17 Thread Boris Ostrovsky 
On 05/17/2018 11:02 AM, Jan Beulich wrote:
 On 17.05.18 at 16:47,  wrote:
>> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>>  mov %eax,%es
>>  mov %eax,%ss
>>  
>> +mov $PVH_CANARY_SEL,%eax
>> +mov %eax,%gs
> I doubt this is needed for 64-bit (you could equally well load zero or leave
> in place what's there in that case),

I don't understand this.


>  and loading the selector before setting
> the base address in the descriptor won't have the intended effect.


I wasn't sure about this either but then I noticed that
secondary_startup_64() does it in the same order (although not using the
MSR).


>
>> @@ -150,9 +170,12 @@ gdt_start:
>>  .quad GDT_ENTRY(0xc09a, 0, 0xf) /* __KERNEL_CS */
>>  #endif
>>  .quad GDT_ENTRY(0xc092, 0, 0xf) /* __KERNEL_DS */
>> +.quad GDT_ENTRY(0x4090, 0, 0x18)/* PVH_CANARY_SEL */
>>  gdt_end:
>>  
>> -.balign 4
>> +.balign 16
>> +canary:
>> +.fill 24, 1, 0
> This is too little space for 64-bit afaict (the canary lives at offset 40 
> there
> if I can trust asm/processor.h).

Yes, should be 48. I didn't realize the two modes use different offsets.

-boris

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

2018-05-17 Thread Jan Beulich 
>>> On 17.05.18 at 16:47,  wrote:
> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>   mov %eax,%es
>   mov %eax,%ss
>  
> + mov $PVH_CANARY_SEL,%eax
> + mov %eax,%gs

I doubt this is needed for 64-bit (you could equally well load zero or leave
in place what's there in that case), and loading the selector before setting
the base address in the descriptor won't have the intended effect.

> @@ -150,9 +170,12 @@ gdt_start:
>   .quad GDT_ENTRY(0xc09a, 0, 0xf) /* __KERNEL_CS */
>  #endif
>   .quad GDT_ENTRY(0xc092, 0, 0xf) /* __KERNEL_DS */
> + .quad GDT_ENTRY(0x4090, 0, 0x18)/* PVH_CANARY_SEL */
>  gdt_end:
>  
> - .balign 4
> + .balign 16
> +canary:
> + .fill 24, 1, 0

This is too little space for 64-bit afaict (the canary lives at offset 40 there
if I can trust asm/processor.h).

Jan

Re: [PATCH v3 1/2] xen/PVH: Set up GS segment for stack canary

2018-05-17 Thread Jan Beulich 
>>> On 17.05.18 at 16:47,  wrote:
> @@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
>   mov %eax,%es
>   mov %eax,%ss
>  
> + mov $PVH_CANARY_SEL,%eax
> + mov %eax,%gs

I doubt this is needed for 64-bit (you could equally well load zero or leave
in place what's there in that case), and loading the selector before setting
the base address in the descriptor won't have the intended effect.

> @@ -150,9 +170,12 @@ gdt_start:
>   .quad GDT_ENTRY(0xc09a, 0, 0xf) /* __KERNEL_CS */
>  #endif
>   .quad GDT_ENTRY(0xc092, 0, 0xf) /* __KERNEL_DS */
> + .quad GDT_ENTRY(0x4090, 0, 0x18)/* PVH_CANARY_SEL */
>  gdt_end:
>  
> - .balign 4
> + .balign 16
> +canary:
> + .fill 24, 1, 0

This is too little space for 64-bit afaict (the canary lives at offset 40 there
if I can trust asm/processor.h).

Jan