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:49, Juergen Gross wrote:
> On 09/04/18 12:46, Andrew Cooper wrote:
>> 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.
> Add a way to include x86_32.h defining the needed COMPAT_* macros only?

For the same argument as the 64bit side, x86_32.h shouldn't contain
COMPAT_* defines, as they are inapplicable.

Furthermore, the constants can't be shared in such an include, because
of naming conflicts with x86_64.h.

~Andrew

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