linux-kernel - Re: [PATCH 03/11] KVM: nSVM: Add missing consistency check for event

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] [day] [month] [year] [list]

Message-ID: <aQv3Ml60dVpQ-fvz@google.com>
Date: Wed, 5 Nov 2025 17:17:38 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Jim Mattson <jmattson@...gle.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 03/11] KVM: nSVM: Add missing consistency check for event_inj

On Wed, Nov 05, 2025, Yosry Ahmed wrote:
> On Wed, Nov 05, 2025 at 10:48:28AM -0800, Sean Christopherson wrote:
> > On Tue, Nov 04, 2025, Yosry Ahmed wrote:
> > > According to the APM Volume #2, 15.20 (24593—Rev. 3.42—March 2024):
> > > 
> > >   VMRUN exits with VMEXIT_INVALID error code if either:
> > >   • Reserved values of TYPE have been specified, or
> > >   • TYPE = 3 (exception) has been specified with a vector that does not
> > >     correspond to an exception (this includes vector 2, which is an NMI,
> > >     not an exception).
> > > 
> > > Add the missing consistency checks to KVM. For the second point, inject
> > > VMEXIT_INVALID if the vector is anything but the vectors defined by the
> > > APM for exceptions. Reserved vectors are also considered invalid, which
> > > matches the HW behavior.
> > 
> > Ugh.  Strictly speaking, that means KVM needs to match the capabilities of the
> > virtual CPU.  E.g. if the virtual CPU predates SEV-ES, then #VC should be reserved
> > from the guest's perspective.
> > 
> > > Vector 9 (i.e. #CSO) is considered invalid because it is reserved on modern
> > > CPUs, and according to LLMs no CPUs exist supporting SVM and producing #CSOs.
> > > 
> > > Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> > > ---
> > >  arch/x86/include/asm/svm.h |  5 +++++
> > >  arch/x86/kvm/svm/nested.c  | 33 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 38 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> > > index e69b6d0dedcf0..3a9441a8954f3 100644
> > > --- a/arch/x86/include/asm/svm.h
> > > +++ b/arch/x86/include/asm/svm.h
> > > @@ -633,6 +633,11 @@ static inline void __unused_size_checks(void)
> > >  #define SVM_EVTINJ_VALID (1 << 31)
> > >  #define SVM_EVTINJ_VALID_ERR (1 << 11)
> > >  
> > > +/* Only valid exceptions (and not NMIs) are allowed for SVM_EVTINJ_TYPE_EXEPT */
> > > +#define SVM_EVNTINJ_INVALID_EXEPTS (NMI_VECTOR | BIT_ULL(9) | BIT_ULL(15) | \
> > > +				    BIT_ULL(20) | GENMASK_ULL(27, 22) | \
> > > +				    BIT_ULL(31))
> > 
> > As above, hardcoding this won't work.  E.g. if a VM is migrated from a CPU where
> > vector X is reserved to a CPU where vector X is valid, then the VM will observe
> > a change in behavior. 
> > 
> > Even if we're ok being overly permissive today (e.g. by taking an erratum), this
> > will create problems in the future when one of the reserved vectors is defined,
> > at which point we'll end up changing guest-visible behavior (and will have to
> > take another erratum, or maybe define the erratum to be that KVM straight up
> > doesn't enforce this correctly?)
> > 
> > And if we do throw in the towel and don't try to enforce this, we'll still want
> > a safeguard against this becoming stale, e.g. when KVM adds support for new
> > feature XYZ that comes with a new vector.
> > 
> > Off the cuff, the best idea I have is to define the positive set of vectors
> > somewhere common with a static assert, and then invert that.  E.g. maybe something
> > shared with kvm_trace_sym_exc()?
> 
> Do you mean define the positive set of vectors dynamically based on the
> vCPU caps? Like a helper returning a dynamic bitmask instead of
> SVM_EVNTINJ_INVALID_EXEPTS?

Ya, that would be option #1, though I'm not entirely sure it's a good option.
The validity of vectors aren't architecturally tied to the existince of any
particular feature, at least not explicitly.  For the "newer" vectors, i.e. the
ones that we can treat as conditionally valid, it's pretty obvious which features
they "belong" to, but even then I hesitate to draw connections, e.g. on the off
chance that some weird hypervisor checks Family/Model/Stepping or something.

> If we'll reuse that for kvm_trace_sym_exc() it will need more work, but
> I don't see why we need a dynamic list for kvm_trace_sym_exc().

Sorry, this is for option #2.  Hardcode the set of vectors that KVM allows (to
prevent L1 from throwing pure garbage at hardware), but otherwise defer to the
CPU to enforce the reserved vectors.

Hrm, but option #2 just delays the inevitable, e.g. we'll be stuck once again
when KVM supports some new vector, in which case we'll have to change guest
visible behavior _again_, or bite the bullet and do option #1.

So I guess do option #1 straight away and hope nothing breaks?  Maybe hardcode
everything as supported except #CP (SHSTK) and #VC (SEV-ES)?

> So my best guess is that I didn't really understand your suggestion :)