| 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
| ||
|
Message-ID: <Zjpkl0U23qEOO3DY@google.com>
Date: Tue, 7 May 2024 10:27:51 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Weijiang Yang <weijiang.yang@...el.com>
Cc: pbonzini@...hat.com, dave.hansen@...el.com, x86@...nel.org,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org, peterz@...radead.org,
chao.gao@...el.com, rick.p.edgecombe@...el.com, mlevitsk@...hat.com,
john.allen@....com
Subject: Re: [PATCH v10 17/27] KVM: x86: Report KVM supported CET MSRs as to-be-saved
On Mon, May 06, 2024, Weijiang Yang wrote:
> On 5/2/2024 6:40 AM, Sean Christopherson wrote:
> > On Sun, Feb 18, 2024, Yang Weijiang wrote:
> > > Add CET MSRs to the list of MSRs reported to userspace if the feature,
> > > i.e. IBT or SHSTK, associated with the MSRs is supported by KVM.
> > >
> > > SSP can only be read via RDSSP. Writing even requires destructive and
> > > potentially faulting operations such as SAVEPREVSSP/RSTORSSP or
> > > SETSSBSY/CLRSSBSY. Let the host use a pseudo-MSR that is just a wrapper
> > > for the GUEST_SSP field of the VMCS.
> > >
> > > Suggested-by: Chao Gao <chao.gao@...el.com>
> > > Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
> > > ---
> > > arch/x86/include/uapi/asm/kvm_para.h | 1 +
> > > arch/x86/kvm/vmx/vmx.c | 2 ++
> > > arch/x86/kvm/x86.c | 18 ++++++++++++++++++
> > > 3 files changed, 21 insertions(+)
> > >
> > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > index 605899594ebb..9d08c0bec477 100644
> > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > @@ -58,6 +58,7 @@
> > > #define MSR_KVM_ASYNC_PF_INT 0x4b564d06
> > > #define MSR_KVM_ASYNC_PF_ACK 0x4b564d07
> > > #define MSR_KVM_MIGRATION_CONTROL 0x4b564d08
> > > +#define MSR_KVM_SSP 0x4b564d09
> > We never resolved the conservation from v6[*], but I still agree with Maxim's
> > view that defining a synthetic MSR, which "steals" an MSR from KVM's MSR address
> > space, is a bad idea.
> >
> > And I still also think that KVM_SET_ONE_REG is the best way forward. Completely
> > untested, but I think this is all that is needed to wire up KVM_{G,S}ET_ONE_REG
> > to support MSRs, and carve out room for 250+ other register types, plus room for
> > more future stuff as needed.
>
> Got your point now.
>
> >
> > We'll still need a KVM-defined MSR for SSP, but it can be KVM internal, not uAPI,
> > e.g. the "index" exposed to userspace can simply be '0' for a register type of
> > KVM_X86_REG_SYNTHETIC_MSR, and then the translated internal index can be any
> > value that doesn't conflict.
>
> Let me try to understand it, for your reference code below, id.type is to separate normal
> MSR (HW defined) namespace and synthetic MSR namespace, right?
Yep.
> For the latter, IIUC KVM still needs to expose the index within the synthetic
> namespace so that userspace can read/write the intended MSRs, of course not
> expose the synthetic MSR index via existing uAPI, But you said the "index"
> exposed to userspace can simply be '0' in this case, then how to distinguish
> the synthetic MSRs in userspace and KVM? And how userspace can be aware of
> the synthetic MSR index allocation in KVM?
The idea is to have a synthetic index that is exposed to userspace, and a separate
KVM-internal index for emulating accesses. The value that is exposed to userspace
can start at 0 and be a simple incrementing value as we add synthetic MSRs, as the
type == SYNTHETIC makes it impossible for the value to collide with a "real" MSR.
Translating to a KVM-internal index is a hack to avoid having to plumb a 64-bit
index into all the MSR code. We could do that, i.e. pass the full kvm_x86_reg_id
into the MSR helpers, but I'm not convinced it'd be worth the churn. That said,
I'm not opposed to the idea either, if others prefer that approach.
E.g.
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 738c449e4f9e..21152796238a 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -420,6 +420,8 @@ struct kvm_x86_reg_id {
__u16 rsvd16;
};
+#define MSR_KVM_GUEST_SSP 0
+
#define KVM_SYNC_X86_REGS (1UL << 0)
#define KVM_SYNC_X86_SREGS (1UL << 1)
#define KVM_SYNC_X86_EVENTS (1UL << 2)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f45cdd9d8c1f..1a9e1e0c9f49 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5990,6 +5990,19 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
}
}
+static int kvm_translate_synthetic_msr(u32 *index)
+{
+ switch (*index) {
+ case MSR_KVM_GUEST_SSP:
+ *index = MSR_KVM_INTERNAL_GUEST_SSP;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
long kvm_arch_vcpu_ioctl(struct file *filp,
unsigned int ioctl, unsigned long arg)
{
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index cc585051d24b..3b5a038f5260 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -49,6 +49,15 @@ void kvm_spurious_fault(void);
#define KVM_FIRST_EMULATED_VMX_MSR MSR_IA32_VMX_BASIC
#define KVM_LAST_EMULATED_VMX_MSR MSR_IA32_VMX_VMFUNC
+/*
+ * KVM's internal, non-ABI indices for synthetic MSRs. The values themselves
+ * are arbitrary and have no meaning, the only requirement is that they don't
+ * conflict with "real" MSRs that KVM supports. Use values at the uppper end
+ * of KVM's reserved paravirtual MSR range to minimize churn, i.e. these values
+ * will be usable until KVM exhausts its supply of paravirtual MSR indices.
+ */
+#define MSR_KVM_INTERNAL_GUEST_SSP 0x4b564dff
+
#define KVM_DEFAULT_PLE_GAP 128
#define KVM_VMX_DEFAULT_PLE_WINDOW 4096
#define KVM_DEFAULT_PLE_WINDOW_GROW 2
Powered by blists - more mailing lists