Re: [PATCH V3 1/6] x86/xsaves: enable xsaves/xrstors for pv guest

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

On 11/08/15 08:50, Shuai Ruan wrote:
> On Fri, Aug 07, 2015 at 01:44:41PM +0100, Andrew Cooper wrote:
>> On 07/08/15 09:00, Shuai Ruan wrote:
>>>>> +                    goto skip;
>>>>> +                }
>>>>> +
>>>>> +                if ( !guest_kernel_mode(v, regs) || (regs->edi & 0x3f) )
>>>> What does edi have to do with xsaves?  only edx:eax are special
>>>> according to the manual.
>>>>
>>> regs->edi is the guest_linear_address
>> Whyso?  xsaves takes an unconditional memory parameter,  not a pointer
>> in %rdi.  (regs->edi is only correct for ins/outs because the pointer is
>> architecturally required to be in %rdi.)
> You are right. The linear_address should be decoded from the instruction.
>> There is nothing currently in emulate_privileged_op() which does ModRM
>> decoding for memory references, nor SIB decoding.  xsaves/xrstors would
>> be the first such operations.
>>
>> I am also not sure that adding arbitrary memory decode here is sensible.
>>
>> In an ideal world, we would have what is currently x86_emulate() split
>> in 3 stages.
>>
>> Stage 1 does straight instruction decode to some internal representation.
>>
>> Stage 2 does an audit to see whether the decoded instruction is
>> plausible for the reason why an emulation was needed.  We have had a
>> number of security issues with emulation in the past where guests cause
>> one instruction to trap for emulation, then rewrite the instruction to
>> be something else, and exploit a bug in the emulator.
>>
>> Stage 3 performs the actions required for emulation.
>>
>> Currently, x86_emulate() is limited to instructions which might
>> legitimately fault for emulation, but with the advent of VM
>> introspection, this is proving to be insufficient.  With my x86
>> maintainers hat on, I would like to avoid the current situation we have
>> with multiple bits of code doing x86 instruction decode and emulation
>> (which are all different).
>>
>> I think the 3-step approach above caters suitably to all usecases, but
>> it is a large project itself.  It allows the introspection people to
>> have a full and complete x86 emulation infrastructure, while also
>> preventing areas like the shadow paging from being opened up to
>> potential vulnerabilities in unrelated areas of the x86 architecture.
>>
>> I would even go so far as to say that it is probably ok not to support
>> xsaves/xrestors in PV guests until something along the above lines is
>> sorted.  The first feature in XSS is processor trace which a PV guest
>> couldn't use anyway.  I suspect the same applies to most of the other
> Why PV guest couldn't use precessor trace?

After more consideration, Xen should not expose xsaves/xrstors to PV
guests at all.

>> XSS features, or they wouldn't need to be privileged in the first place.
>>
> Thanks for your such detail suggestions.
> For xsaves/xrstors would also bring other benefits for PV guest such as
> saving memory of XSAVE area. If we do not support xsaves/xrstors in PV , 
> PV guest would lose these benefits. What's your opinions toward this?

PV guests running under Xen are exactly the same as regular user
processes running under Linux.

There is a reason everything covered by xsaves/xrstors is restricted to
ring0; it would be a security hole to allow guests to configure the
features themselves.

Features such as Processor Trace would need a hypercall interface for
guests to use.

~Andrew