linux-kernel - Re: [PATCH v5 09/19] KVM:x86: Make guest supervisor states as non-XSAVE managed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cf97cfba-941a-5a77-6591-fa84ef6fe8d1@redhat.com>
Date:   Fri, 4 Aug 2023 23:32:00 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Weijiang Yang <weijiang.yang@...el.com>
Cc:     Chao Gao <chao.gao@...el.com>, peterz@...radead.org,
        john.allen@....com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, rick.p.edgecombe@...el.com,
        binbin.wu@...ux.intel.com
Subject: Re: [PATCH v5 09/19] KVM:x86: Make guest supervisor states as
 non-XSAVE managed

On 8/4/23 22:45, Sean Christopherson wrote:
>>>> +void save_cet_supervisor_ssp(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	if (unlikely(guest_can_use(vcpu, X86_FEATURE_SHSTK))) {
> Drop the unlikely, KVM should not speculate on the guest configuration or underlying
> hardware.

In general unlikely() can still be a good idea if you have a fast path 
vs. a slow path; the extra cost of a branch will be much more visible on 
the fast path.  That said the compiler should already be doing that.

>  the Pros:
>   - Super easy to implement for KVM.
>   - Automatically avoids saving and restoring this data when the vmexit
>     is handled within KVM.
>
>  the Cons:
>   - Unnecessarily restores XFEATURE_CET_KERNEL when switching to
>     non-KVM task's userspace.
>   - Forces allocating space for this state on all tasks, whether or not
>     they use KVM, and with likely zero users today and the near future.
>   - Complicates the FPU optimization thinking by including things that
>     can have no affect on userspace in the FPU

I'm not sure if Linux will ever use XFEATURE_CET_KERNEL.  Linux does not 
use MSR_IA32_PL{1,2}_SSP; MSR_IA32_PL0_SSP probably would be per-CPU but 
it is not used while in ring 0 (except for SETSSBSY) and the restore can 
be delayed until return to userspace.  It is not unlike the SYSCALL MSRs.

So I would treat the bit similar to the dynamic features even if it's 
not guarded by XFD, for example

#define XFEATURE_MASK_USER_DYNAMIC XFEATURE_MASK_XTILE_DATA
#define XFEATURE_MASK_USER_OPTIONAL \
	(XFEATURE_MASK_DYNAMIC | XFEATURE_MASK_CET_KERNEL)

where XFEATURE_MASK_USER_DYNAMIC is used for xfd-related tasks but 
everything else uses XFEATURE_MASK_USER_OPTIONAL.

Then you'd enable the feature by hand when allocating the guest fpstate.

> Especially because another big negative is that not utilizing XSTATE bleeds into
> KVM's ABI.  Userspace has to be told to manually save+restore MSRs instead of just
> letting KVM_{G,S}ET_XSAVE handle the state.  And that will create a bit of a
> snafu if Linux does gain support for SSS.

I don't think this matters, we don't have any MSRs in KVM_GET/SET_XSAVE 
and in fact we can't even add them since the uABI uses the non-compacted 
format.  MSRs should be retrieved and set via KVM_GET/SET_MSR and 
userspace will learn about the index automatically via 
KVM_GET_MSR_INDEX_LIST.

Paolo