Re: [Xen-devel] [PATCH for-4.11] x86/pv: Fix up erroneous segments for 32bit syscall entry

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/04/18 11:44, Wei Liu wrote:
> On Mon, Apr 09, 2018 at 10:44:47AM +0100, Andrew Cooper wrote:
>> The existing FLAT_KERNEL_SS expands to the correct value, 0xe02b, but is the
>> wrong constant to use.  Switch to FLAT_USER_SS32.
>>
>> For compat domains however, the reported values are entirely bogus.
>> FLAT_USER_SS32 (value 0xe02b) is FLAT_RING3_CS in the 32bit ABI, while
>> FLAT_USER_CS32 (value 0xe023) is FLAT_RING1_DS with an RPL of 3.
>>
>> The guests SYSCALL callback is invoked with a broken iret frame, and if left
>> unmodified by the guest, will fail on the way back out when Xen's iret tries
>> to load a code segment into %ss.
>>
>> In practice, this is only a problem for 32bit PV guests on AMD hardware, as
>> Intel hardware doesn't permit the SYSCALL instruction outside of 64bit mode.
>>
>> This appears to have been broken ever since 64bit support was added to Xen,
>> and has gone unnoticed because Linux doesn't use SYSCALL in 32bit builds.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>>
>> This wants backporting basically everywhere, and as such, also wants to be
>> considered for 4.11 at this point.
>> ---
>>  xen/arch/x86/x86_64/compat/entry.S | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/x86_64/compat/entry.S 
>> b/xen/arch/x86/x86_64/compat/entry.S
>> index 6c7fcf9..2bc046c 100644
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -197,7 +197,7 @@ ENTRY(cstar_enter)
>>          /* sti could live here when we don't switch page tables below. */
>>          CR4_PV32_RESTORE
>>          movq  8(%rsp),%rax /* Restore %rax. */
>> -        movq  $FLAT_KERNEL_SS,8(%rsp)
>> +        movq  $FLAT_USER_SS32, 8(%rsp) /* Assume a 64bit domain.  Compat 
>> handled lower. */
>>          pushq %r11
>>          pushq $FLAT_USER_CS32
>>          pushq %rcx
>> @@ -223,6 +223,11 @@ ENTRY(cstar_enter)
>>          movq  VCPU_domain(%rbx),%rcx
>>          cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
>>          je    switch_to_kernel
>> +
>> +        /* Fix up reported %cs/%ss for compat domains. */
>> +        movl  $0xe033, UREGS_ss(%rsp) /* Compat FLAT_RING3_SS */
>> +        movl  $0xe02b, UREGS_cs(%rsp) /* Compat FLAT_RING3_CS */
> I wonder if it would be better to introduce COMPAT_FLAT_RING3_* in
> xen-x86_64.h?
>
> In any case, the reasoning and code looks correct to me:
>
> Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>

I considered that, and am open to suggestions.  The problem is that we
don't want to put COMPAT definitions in that header file, because they
are inapplicable to 64bit PV guests, who are intended consumers.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel