Re: [Xen-devel] [PATCH 7/7] x86emul: support SYSRET

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 25/03/2020 10:19, Jan Beulich wrote:
> On 25.03.2020 11:00, Andrew Cooper wrote:
>> On 24/03/2020 16:29, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -5975,6 +5975,60 @@ x86_emulate(
>>>              goto done;
>>>          break;
>>>  
>>> +    case X86EMUL_OPC(0x0f, 0x07): /* sysret */
>>> +        vcpu_must_have(syscall);
>>> +        /* Inject #UD if syscall/sysret are disabled. */
>>> +        fail_if(!ops->read_msr);
>>> +        if ( (rc = ops->read_msr(MSR_EFER, &msr_val, ctxt)) != X86EMUL_OKAY )
>>> +            goto done;
>>> +        generate_exception_if((msr_val & EFER_SCE) == 0, EXC_UD);
>> (as with the SYSCALL side), no need for the vcpu_must_have(syscall) as
>> well as this check.
> Hmm, yes, we do so elsewhere too, so I'll adjust this there and here.

In theory, the SEP checks for SYSENTER/SYSEXIT could be similarly
dropped, once the MSR logic is updated to perform proper availability
checks.

>>> +        if ( (rc = ops->read_msr(MSR_STAR, &msr_val, ctxt)) != X86EMUL_OKAY )
>>> +            goto done;
>>> +        sreg.sel = ((msr_val >> 48) + 8) | 3; /* SELECTOR_RPL_MASK */
>> This would be the logical behaviour...
>>
>> AMD CPUs |3 into %cs.sel, but don't make an equivalent adjustment for
>> %ss.sel, and simply take MSR_START.SYSRET_CS + 8.
>>
>> If you aren't careful with MSR_STAR, SYSRET will return to userspace
>> with mismatching RPL/DPL and userspace can really find itself with an
>> %ss with an RPL of 0.  (Of course, when you take an interrupt and
>> attempt to IRET back to this context, things fall apart).
>>
>> I discovered this entirely by accident in XTF, but it is confirmed by
>> careful reading of the AMD SYSRET pseudocode.
> I did notice this in their pseudocode, but it looked too wrong to
> be true. Will change.

The main reason why my 204 followon series is still pending is because I
never got around to completing an XTF test for all of these corner cases.

I'm happy to drop my series to Xen in light of this series of yours, but
I'd still like to complete the XTF side of things at some point.

>>> +
>>> +#ifdef __x86_64__
>>> +        if ( mode_64bit() )
>>> +        {
>>> +            if ( op_bytes == 8 )
>>> +            {
>>> +                cs.attr = 0xafb; /* L+DB+P+DPL3+S+Code */
>>> +                generate_exception_if(!is_canonical_address(_regs.rcx) &&
>>> +                                      !amd_like(ctxt), EXC_GP, 0);
>> Wherever this ends up living, I think it needs calling out with a
>> comment /* CVE-xxx, Intel privilege escalation hole */, as it is a very
>> subtle piece of vendor specific behaviour.
>>
>> Do we have a Centaur/other CPU to try with?  I'd err on the side of
>> going with == Intel rather than !AMD to avoid introducing known
>> vulnerabilities into models which stand half a chance of not being affected.
> I'd rather not - this exception behavior is spelled out by the
> SDM, and hence imo pretty likely to be followed by clones.

In pseudocode which certainly used to state somewhere "for reference
only, and not to be taken as an precise specification of behaviour". 
(And yes - that statement was still at the beginning of Vol2 when Intel
also claimed that "SYSRET was working according to the spec" in the
embargo period of XSA-7, because I called them out on it).

And anyway - it is a part of the AMD64 spec, not the Intel32 spec.  A
3rd party implementing it for 64bit support is more likely to go with
AMD's writings of how it behaves.

> While I do have a VIA box somewhere, it's not stable enough to
> run for more than a couple of minutes.

Fundamentally, it boils down to this.

Intel behaviour leaves a privilege escalation vulnerability available to
userspace.

Assuming AMD behaviour for unknown parts is the safer course of action,
because we don't need to issue an XSA/CVE to fix the emulator when it
turns out that we're wrong.

~Andrew