linux-kernel - Re: [PATCH v15 13/41] KVM: x86: Enable guest SSP read/write interface with new uAPIs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aMiPTEu_WfmEZiqT@google.com>
Date: Mon, 15 Sep 2025 15:12:28 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Xiaoyao Li <xiaoyao.li@...el.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Tom Lendacky <thomas.lendacky@....com>, Mathias Krause <minipli@...ecurity.net>, 
	John Allen <john.allen@....com>, Rick Edgecombe <rick.p.edgecombe@...el.com>, 
	Chao Gao <chao.gao@...el.com>, Maxim Levitsky <mlevitsk@...hat.com>, 
	Zhang Yi Z <yi.z.zhang@...ux.intel.com>
Subject: Re: [PATCH v15 13/41] KVM: x86: Enable guest SSP read/write interface
 with new uAPIs

On Mon, Sep 15, 2025, Xiaoyao Li wrote:
> On 9/13/2025 7:22 AM, Sean Christopherson wrote:
> > @@ -6097,11 +6105,22 @@ static int kvm_get_set_one_reg(struct kvm_vcpu *vcpu, unsigned int ioctl,
> >   static int kvm_get_reg_list(struct kvm_vcpu *vcpu,
> >   			    struct kvm_reg_list __user *user_list)
> >   {
> > -	u64 nr_regs = 0;
> > +	u64 nr_regs = guest_cpu_cap_has(vcpu, X86_FEATURE_SHSTK) ? 1 : 0;
> 
> I wonder what's the semantic of KVM returning KVM_REG_GUEST_SSP on
> KVM_GET_REG_LIST. Does it ensure KVM_{G,S}ET_ONE_REG returns -EINVAL on
> KVM_REG_GUEST_SSP when it's not enumerated by KVM_GET_REG_LIST?
> 
> If so, but KVM_{G,S}ET_ONE_REG can succeed on GUEST_SSP even if
> !guest_cpu_cap_has() when @ignore_msrs is true.

Ugh, great catch.  Too many knobs.  The best idea I've got it to to exempt KVM-
internal MSRs from ignore_msrs and report_ignored_msrs on host-initiated writes.
That's unfortunately still a userspace visible change, and would continue to be
userspace-visible, e.g. if we wanted to change the magic value for
MSR_KVM_INTERNAL_GUEST_SSP.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c78acab2ff3f..6a50261d1c5c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -511,6 +511,11 @@ static bool kvm_is_advertised_msr(u32 msr_index)
        return false;
 }
 
+static bool kvm_is_internal_msr(u32 msr)
+{
+       return msr == MSR_KVM_INTERNAL_GUEST_SSP;
+}
+
 typedef int (*msr_access_t)(struct kvm_vcpu *vcpu, u32 index, u64 *data,
                            bool host_initiated);
 
@@ -544,6 +549,9 @@ static __always_inline int kvm_do_msr_access(struct kvm_vcpu *vcpu, u32 msr,
        if (host_initiated && !*data && kvm_is_advertised_msr(msr))
                return 0;
 
+       if (host_initiated && kvm_is_internal_msr(msr))
+               return ret;
+
        if (!ignore_msrs) {
                kvm_debug_ratelimited("unhandled %s: 0x%x data 0x%llx\n",
                                      op, msr, *data);

Alternatively, simply exempt host writes from ignore_msrs.  Aha!  And KVM even
documents that as the behavior:

	kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
			Default is 0 (don't ignore, but inject #GP)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c78acab2ff3f..177253e75b41 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -544,7 +544,7 @@ static __always_inline int kvm_do_msr_access(struct kvm_vcpu *vcpu, u32 msr,
        if (host_initiated && !*data && kvm_is_advertised_msr(msr))
                return 0;
 
-       if (!ignore_msrs) {
+       if (host_initiated || !ignore_msrs) {
                kvm_debug_ratelimited("unhandled %s: 0x%x data 0x%llx\n",
                                      op, msr, *data);
                return ret;

So while it's technically an ABI change (arguable since it's guarded by an
off-by-default param), I suspect we can get away with it.  Hmm, commit 6abe9c1386e5
("KVM: X86: Move ignore_msrs handling upper the stack") exempted KVM-internal
MSR accesses from ignore_msrs, but doesn't provide much in the way of justification
for _why_ that's desirable.

Argh, and that same mini-series extended the behavior to feature MSRs, again
without seeming to consider whether or not it's actually desirable to suppress
bad VMM accesses.  Even worse, that decision likely generated an absurd amount
of churn and noise due to splattering helpers and variants all over the place. :-(

commit 12bc2132b15e0a969b3f455d90a5f215ef239eff
Author:     Peter Xu <peterx@...hat.com>
AuthorDate: Mon Jun 22 18:04:42 2020 -0400
Commit:     Paolo Bonzini <pbonzini@...hat.com>
CommitDate: Wed Jul 8 16:21:40 2020 -0400

    KVM: X86: Do the same ignore_msrs check for feature msrs
    
    Logically the ignore_msrs and report_ignored_msrs should also apply to feature
    MSRs.  Add them in.

For 6.18, I think the safe play is to go with the first path (exempt KVM-internal
MSRs), and then try to go for the second approach (exempt all host accesses) for
6.19.  KVM's ABI for ignore_msrs=true is already all kinds of messed up, so I'm
not terribly concerned about temporarily making it marginally worse.