linux-kernel - Re: [PATCH V2 2/5] KVM: SVM: Inject #UD for STGI if EFER.SVME=0 and SVM Lock and DEV are not available

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: <CAE6NW_bjcj5mto_pt_tNt3Fysirw7_aG_+c69u-+bZwwWVPgGA@mail.gmail.com>
Date: Thu, 22 Jan 2026 00:02:32 -0500
From: Kevin Cheng <chengkev@...gle.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
Cc: seanjc@...gle.com, pbonzini@...hat.com, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 2/5] KVM: SVM: Inject #UD for STGI if EFER.SVME=0 and
 SVM Lock and DEV are not available

On Mon, Jan 12, 2026 at 3:50 PM Yosry Ahmed <yosry.ahmed@...ux.dev> wrote:
>
> On Mon, Jan 12, 2026 at 05:45:32PM +0000, Kevin Cheng wrote:
> > The AMD APM states that STGI causes a #UD if SVM is not enabled and
> > neither SVM Lock nor the device exclusion vector (DEV) are supported.
>
> Might be useful to also mention the following part "Support for DEV is
> part of the SKINIT architecture".
>
> > Fix the STGI exit handler by injecting #UD when these conditions are
> > met.
> >
> > Signed-off-by: Kevin Cheng <chengkev@...gle.com>
> > ---
> >  arch/x86/kvm/svm/svm.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 6373a25d85479..557c84a060fc6 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -2271,8 +2271,18 @@ static int stgi_interception(struct kvm_vcpu *vcpu)
> >  {
> >       int ret;
> >
> > -     if (nested_svm_check_permissions(vcpu))
> > +     if ((!(vcpu->arch.efer & EFER_SVME) &&
> > +          !guest_cpu_cap_has(vcpu, X86_FEATURE_SVML) &&
> > +          !guest_cpu_cap_has(vcpu, X86_FEATURE_SKINIT)) ||
> > +         !is_paging(vcpu)) {
> > +             kvm_queue_exception(vcpu, UD_VECTOR);
> > +             return 1;
> > +     }
> > +
> > +     if (to_svm(vcpu)->vmcb->save.cpl) {
> > +             kvm_inject_gp(vcpu, 0);
> >               return 1;
> > +     }
>
> Not a big fan of open-coding nested_svm_check_permissions() here. The
> checks could get out of sync.
>
> How about refactoring nested_svm_check_permissions() like so:
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index f295a41ec659..7f53c54b9d39 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1520,9 +1520,10 @@ int nested_svm_exit_handled(struct vcpu_svm *svm)
>         return vmexit;
>  }
>
> -int nested_svm_check_permissions(struct kvm_vcpu *vcpu)
> +static int __nested_svm_check_permissions(struct kvm_vcpu *vcpu,
> +                                         bool insn_allowed)
>  {
> -       if (!(vcpu->arch.efer & EFER_SVME) || !is_paging(vcpu)) {
> +       if (!insn_allowed || !is_paging(vcpu)) {
>                 kvm_queue_exception(vcpu, UD_VECTOR);
>                 return 1;
>         }
> @@ -1535,6 +1536,11 @@ int nested_svm_check_permissions(struct kvm_vcpu *vcpu)
>         return 0;
>  }
>
> +int nested_svm_check_permissions(struct kvm_vcpu *vcpu)
> +{
> +       return __nested_svm_check_permissions(vcpu, vcpu->arch.efer & EFER_SVME);
> +}
> +
>  static bool nested_svm_is_exception_vmexit(struct kvm_vcpu *vcpu, u8 vector,
>                                            u32 error_code)
>  {
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7041498a8091..6340c4ce323c 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2333,9 +2333,19 @@ void svm_set_gif(struct vcpu_svm *svm, bool value)
>
>  static int stgi_interception(struct kvm_vcpu *vcpu)
>  {
> +       bool insn_allowed;
>         int ret;
>
> -       if (nested_svm_check_permissions(vcpu))
> +       /*
> +        * According to the APM, STGI is allowed even with SVM disabled if SVM
> +        * Lock or device exclusion vector (DEV) are supported. DEV is part of
> +        * the SKINIT architecture.
> +        */
> +       insn_allowed = (vcpu->arch.efer & EFER_SVME) ||
> +               guest_cpu_cap_has(vcpu, X86_FEATURE_SVML) ||
> +               guest_cpu_cap_has(vcpu, X86_FEATURE_SKINIT);
> +
> +       if (__nested_svm_check_permissions(vcpu, insn_allowed))
>                 return 1;
>
>         ret = kvm_skip_emulated_instruction(vcpu);
>
> ---
>
> We may also want to rename nested_svm_check_permissions() to
> nested_svm_insn_check_permissions() or something. Sean, WDYT?

I just sent out v3 without the rename for now. Sean, if you prefer
nested_svm_insn_check_permissions over nested_svm_check_permissions
let me know and I can change along with any final revisions.