All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	 Haiyang Zhang <haiyangz@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>,  Dexuan Cui <decui@microsoft.com>,
	kvm@vger.kernel.org, linux-hyperv@vger.kernel.org,
	 linux-kernel@vger.kernel.org, Jim Mattson <jmattson@google.com>,
	 Yosry Ahmed <yosry.ahmed@linux.dev>
Subject: Re: [PATCH 6/9] KVM: SVM: Filter out 64-bit exit codes when invoking exit handlers on bare metal
Date: Wed, 19 Nov 2025 14:05:04 -0800	[thread overview]
Message-ID: <aR4_EM5bWKSQ4iOS@google.com> (raw)
In-Reply-To: <60f7c9b3-312f-41e2-ab47-c4361df1d825@redhat.com>

On Sat, Nov 15, 2025, Paolo Bonzini wrote:
> On 11/13/25 23:56, Sean Christopherson wrote:
> > Explicitly filter out 64-bit exit codes when invoking exit handlers, as
> > svm_exit_handlers[] will never be sized with entries that use bits 63:32.
> > 
> > Processing the non-failing exit code as a 32-bit value will allow tracking
> > exit_code as a single 64-bit value (which it is, architecturally).  This
> > will also allow hardening KVM against Spectre-like attacks without needing
> > to do silly things to avoid build failures on 32-bit kernels
> > (array_index_nospec() rightly asserts that the index fits in an "unsigned
> > long").
> > 
> > Omit the check when running as a VM, as KVM has historically failed to set
> > bits 63:32 appropriately when synthesizing VM-Exits, i.e. KVM could get
> > false positives when running as a VM on an older, broken KVM/kernel.  From
> > a functional perspective, omitting the check is "fine", as any unwanted
> > collision between e.g. VMEXIT_INVALID and a 32-bit exit code will be
> > fatal to KVM-on-KVM regardless of what KVM-as-L1 does.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/kvm/svm/svm.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 202a4d8088a2..3b05476296d0 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3433,8 +3433,22 @@ static void dump_vmcb(struct kvm_vcpu *vcpu)
> >   		sev_free_decrypted_vmsa(vcpu, save);
> >   }
> > -int svm_invoke_exit_handler(struct kvm_vcpu *vcpu, u64 exit_code)
> > +int svm_invoke_exit_handler(struct kvm_vcpu *vcpu, u64 __exit_code)
> >   {
> > +	u32 exit_code = __exit_code;
> > +
> > +	/*
> > +	 * SVM uses negative values, i.e. 64-bit values, to indicate that VMRUN
> > +	 * failed.  Report all such errors to userspace (note, VMEXIT_INVALID,
> > +	 * a.k.a. SVM_EXIT_ERR, is special cased by svm_handle_exit()).  Skip
> > +	 * the check when running as a VM, as KVM has historically left garbage
> > +	 * in bits 63:32, i.e. running KVM-on-KVM would hit false positives if
> > +	 * the underlying kernel is buggy.
> > +	 */
> > +	if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR) &&
> > +	    (u64)exit_code != __exit_code)
> > +		goto unexpected_vmexit;
> 
> I reviewed the series and it looks good, but with respect to this patch and
> patch 8, is it really worth it?  While there is a possibility that code
> 0x00000000ffffffff is used, or that any high 32-bit values other than
> all-zeros or all-ones are used, they'd be presumably enabled by some control
> bits in the VMCB or some paravirt thing in the hypervisor.

Maybe.  E.g. TDCALL and SEAMCALL VM-Exits on Intel show up without any enablement
in software (beyond VMXON).  I completely agree that it's extremely unlikely that
AMD will add a on-negative exit code with bits 63:32 != 0, i.e. that we could get
a false positive when truncating exit_code to a u32, but it also seems harmless
to be paranoid.

FWIW, I was assuming VMEXIT_INVALID_PMC (-4) was a generic vPMU thing, but it
looks like that one is also SEV-ES+ specific.

As for e57b84699534 ("KVM: SVM: Limit incorrect check on SVM_EXIT_ERR to running
as a VM"), I agree that being paranoid probably doesn't do anything in practice,
but I like being consistent. :-)

> What really matters is that SEV-ES's kvm_get_cached_sw_exit_code() is
> reading the full 64 bits and discarding invalid codes before reaching
> svm_invoke_exit_handler().

No?  sev_handle_vmgexit() only handles SVM_VMGEXIT_xxx exit codes, everything
else is punted to svm_invoke_exit_handler()

	exit_code = kvm_get_cached_sw_exit_code(control);
	switch (exit_code) {
	case SVM_VMGEXIT_<0>
	...
	case SVM_VMGEXIT_<N>
	default:
		ret = svm_invoke_exit_handler(vcpu, exit_code);
	}

And I don't see anything that filters/modifies exit_code_hi.

  reply	other threads:[~2025-11-19 22:05 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-11-13 22:56 [PATCH 0/9] KVM: SVM: Fix (hilarious) exit_code bugs Sean Christopherson
2025-11-13 22:56 ` [PATCH 1/9] KVM: nSVM: Clear exit_code_hi in VMCB when synthesizing nested VM-Exits Sean Christopherson
2025-11-13 23:03   ` Yosry Ahmed
2025-11-13 22:56 ` [PATCH 2/9] KVM: nSVM: Set exit_code_hi to -1 when synthesizing SVM_EXIT_ERR (failed VMRUN) Sean Christopherson
2025-11-13 23:17   ` Yosry Ahmed
2025-11-13 23:28   ` Yosry Ahmed
2025-11-13 22:56 ` [PATCH 3/9] KVM: SVM: Add a helper to detect VMRUN failures Sean Christopherson
2025-11-13 23:30   ` Yosry Ahmed
2025-11-13 23:35     ` Sean Christopherson
2025-11-13 22:56 ` [PATCH 4/9] KVM: SVM: Open code handling of unexpected exits in svm_invoke_exit_handler() Sean Christopherson
2025-11-13 23:33   ` Yosry Ahmed
2025-11-13 22:56 ` [PATCH 5/9] KVM: SVM: Check for an unexpected VM-Exit after RETPOLINE "fast" handling Sean Christopherson
2025-11-14  0:04   ` Yosry Ahmed
2025-11-13 22:56 ` [PATCH 6/9] KVM: SVM: Filter out 64-bit exit codes when invoking exit handlers on bare metal Sean Christopherson
2025-11-14  0:06   ` Yosry Ahmed
2025-11-14 23:32   ` Paolo Bonzini
2025-11-19 22:05     ` Sean Christopherson [this message]
2025-11-13 22:56 ` [PATCH 7/9] KVM: SVM: Treat exit_code as an unsigned 64-bit value through all of KVM Sean Christopherson
2025-11-14  0:08   ` Yosry Ahmed
2025-11-14  5:26   ` Michael Kelley
2025-11-14 15:22     ` Sean Christopherson
2025-11-14 18:29       ` Wei Liu
2025-11-14 18:35         ` Sean Christopherson
2025-11-14 18:40           ` Wei Liu
2025-11-14 15:27   ` Sean Christopherson
2025-11-14 15:47     ` Sean Christopherson
2025-11-14 23:33       ` Paolo Bonzini
2025-11-13 22:56 ` [PATCH 8/9] KVM: SVM: Limit incorrect check on SVM_EXIT_ERR to running as a VM Sean Christopherson
2025-11-14  0:11   ` Yosry Ahmed
2025-11-13 22:56 ` [PATCH 9/9] KVM: SVM: Harden exit_code against being used in Spectre-like attacks Sean Christopherson
2025-12-05 16:59 ` [PATCH 0/9] KVM: SVM: Fix (hilarious) exit_code bugs Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aR4_EM5bWKSQ4iOS@google.com \
    --to=seanjc@google.com \
    --cc=decui@microsoft.com \
    --cc=haiyangz@microsoft.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=vkuznets@redhat.com \
    --cc=wei.liu@kernel.org \
    --cc=yosry.ahmed@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.