Re: [PATCH] x86/CPUID: surface suitable value in EBX of XSTATE subleaf 1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2022 12:48, Andrew Cooper wrote:
> On 23/08/2022 10:27, Jan Beulich wrote:
>> On 23.08.2022 10:59, Andrew Cooper wrote:
>>> On 23/08/2022 07:42, Jan Beulich wrote:
>>>> exposed to PV domains.
>>>>
>>>> Considering that the size reported is that of the compacted save area,
>>>> I view Linux'es assumption as appropriate (short of the SDM properly
>>>> considering the case). Therefore we need to populate the field also when
>>>> only XSAVEC is supported for a guest.
>>> This is a mess.  The SDM is fairly clear (but only in Vol1) that this
>>> leaf is specific to XSAVES.
>> The way it's written my assumption is that they simply didn't care about
>> XSAVEC when writing this, or they were assuming that both features would
>> always be supported together (yet even if they are in Intel's hardware,
>> the architecture should spell out things as if both were entirely
>> independent, or it should specify that one takes the other as a prereq).
> 
> Real hardware has XSAVEC == XSAVES on Intel (Skylake) and AMD (Zen1). 
> Despite an attempt to separate the parts of the ISA, they are
> inextricably linked.
> 
> It is only under virt that we get XSAVEC without XSAVES.
> 
>>>> Fixes: 460b9a4b3630 ("x86/xsaves: enable xsaves/xrstors for hvm guest")
>>>> Fixes: 8d050ed1097c ("x86: don't expose XSAVES capability to PV guests")
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> CC Marek.  Looks like Jan has found the issue you reported on IRC.
>>>
>>> Jan: Be aware that I submitted
>>> https://lore.kernel.org/lkml/20220810221909.12768-1-andrew.cooper3@xxxxxxxxxx/
>>> to Linux to correct some of the diagnostics.
>>>> --- a/xen/arch/x86/cpuid.c
>>>> +++ b/xen/arch/x86/cpuid.c
>>>> @@ -1142,7 +1142,7 @@ void guest_cpuid(const struct vcpu *v, u
>>>>          switch ( subleaf )
>>>>          {
>>>>          case 1:
>>>> -            if ( p->xstate.xsaves )
>>>> +            if ( p->xstate.xsavec || p->xstate.xsaves )
>>> If we're doing this, then it wants to be xsavec only, with the comment
>>> being extended to explain why.
>> Why would that be? Both insns use compacted format, and neither is
>> dependent upon the other in terms of being supported. IOW XSAVES alone
>> and XSAVEC alone enabled for a domain should still lead through this
>> path.
> 
> Hmm.  Because my fixes to compaction handling haven't been committed
> yet, and in particular one the one which makes XSAVES strictly depend on
> XSAVEC.
> 
> In which case this hunk is correct for Xen as it currently is, and will
> be need to be adjusted when I rebase the compaction series.

May I translate this to an Ack then? Iirc there were no other change
requests.

>>> But this is going to further complicate my several-year-old series
>>> trying to get Xen's XSTATE handling into a position where we can start
>>> to offer supervisor states.
>> Where do you see further complication? The necessary fiddling with XSS
>> here would of course be dependent upon p->xstate.xsaves alone (or,
>> maybe better, on the set of enabled features in XSS being non-empty),
>> but that's simply another (inner) if().
>>
>> As an aside, I actually wonder what use the supplied size is to user
>> mode code when any XSS-controlled feature is enabled: They'd allocate
>> a needlessly large block of memory, as they would only be able to use
>> XSAVEC.
> 
> This field is an already known kernel=>user infoleak.  There are threads
> about it on LKML.
> 
> But it does highlight another problem.  This change does not fix Linux
> on AMD Zen3 hardware, where the kernel will find the CPUID value larger
> than it can calculate the size to be, because Xen's use of CET-SS will
> show up in the CPUID value.
> 
> Linux needs an adjustment from != to <= for this check.

I was wondering about that too, but if I'm not mistaken the change you
suggest is the opposite of what would be apparently safe there (against
overrunning buffers). Hence it may take more than just the comparison
type to be modified.

Jan