linux-kernel - Re: [PATCH v2 6/8] KVM: x86: nSVM: Save/restore gPAT with KVM_{GET,SET}_NESTED

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <wjetkt5qasq2reylgmu6hsrifkoh7drmu5655xoqyjowjlncri@j6aipt5y4mpb>
Date: Thu, 22 Jan 2026 01:51:10 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Jim Mattson <jmattson@...gle.com>
Cc: Sean Christopherson <seanjc@...gle.com>, 
	Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, 
	Shuah Khan <shuah@...nel.org>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 6/8] KVM: x86: nSVM: Save/restore gPAT with
 KVM_{GET,SET}_NESTED_STATE

On Thu, Jan 15, 2026 at 08:23:26PM -0800, Jim Mattson wrote:
> On Thu, Jan 15, 2026 at 3:22 PM Jim Mattson <jmattson@...gle.com> wrote:
> >
> > Add a 'flags' field to the SVM nested state header, and use bit 0 of the
> > flags to indicate that gPAT is stored in the nested state.
> >
> > If in guest mode with NPT enabled, store the current vmcb->save.g_pat value
> > into the vmcb save area of the nested state, and set the flag.
> >
> > Note that most of the vmcb save area in the nested state is populated with
> > dead (and potentially already clobbered) vmcb01 state. A few fields hold L1
> > state to be restored at VMEXIT. Previously, the g_pat field was in the
> > former category.
> >
> > Also note that struct kvm_svm_nested_state_hdr is included in a union
> > padded to 120 bytes, so there is room to add the flags field without
> > changing any offsets.
> >
> > Signed-off-by: Jim Mattson <jmattson@...gle.com>
> > ---
> >  arch/x86/include/uapi/asm/kvm.h |  3 +++
> >  arch/x86/kvm/svm/nested.c       | 13 ++++++++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index 7ceff6583652..80157b9597db 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -495,6 +495,8 @@ struct kvm_sync_regs {
> >
> >  #define KVM_STATE_VMX_PREEMPTION_TIMER_DEADLINE        0x00000001
> >
> > +#define KVM_STATE_SVM_VALID_GPAT       BIT(0)
> > +
> >  /* vendor-independent attributes for system fd (group 0) */
> >  #define KVM_X86_GRP_SYSTEM             0
> >  #  define KVM_X86_XCOMP_GUEST_SUPP     0
> > @@ -530,6 +532,7 @@ struct kvm_svm_nested_state_data {
> >
> >  struct kvm_svm_nested_state_hdr {
> >         __u64 vmcb_pa;
> > +       __u32 flags;
> >  };
> >
> >  /* for KVM_CAP_NESTED_STATE */
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 5fb31faf2b46..c50fb7172672 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1789,6 +1789,8 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
> >         /* First fill in the header and copy it out.  */
> >         if (is_guest_mode(vcpu)) {
> >                 kvm_state.hdr.svm.vmcb_pa = svm->nested.vmcb12_gpa;
> > +               if (nested_npt_enabled(svm))
> > +                       kvm_state.hdr.svm.flags |= KVM_STATE_SVM_VALID_GPAT;
> >                 kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
> >                 kvm_state.flags |= KVM_STATE_NESTED_GUEST_MODE;
> >
> > @@ -1823,6 +1825,11 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
> >         if (r)
> >                 return -EFAULT;
> >
> > +       /*
> > +        * vmcb01->save.g_pat is dead now, so it is safe to overwrite it with
> > +        * vmcb02->save.g_pat, whether or not nested NPT is enabled.
> > +        */
> > +       svm->vmcb01.ptr->save.g_pat = svm->vmcb->save.g_pat;
> 
> Is this too disgusting? Should I extend the payload by 8 bytes
> instead? It seems like such a waste, since most of the save area is
> dead/unused. Maybe I could define a new sparse save state structure,
> with the ~200 bytes that are currently used, surrounded by padding for
> the other 500+ bytes. Then, I could just grab 8 bytes of the padding,
> and it wouldn't seem quite as hacky .

I think this would be cleaner than reusing the vmcb01 field.

One question though, if we decide to start doing save/restore for one of
the save area fields in vmcb01 in the currently unused 500+ bytes (i.e.
the padding), would this be a problem? IIUC the 8 bytes we'll use for
gPAT will overlap with an existing unused field.

> 
> >         if (copy_to_user(&user_vmcb->save, &svm->vmcb01.ptr->save,
> >                          sizeof(user_vmcb->save)))
> >                 return -EFAULT;
> > @@ -1904,7 +1911,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> >                 goto out_free;
> >
> >         /*
> > -        * Validate host state saved from before VMRUN (see
> > +        * Validate host state saved from before VMRUN and gPAT (see
> >          * nested_svm_check_permissions).
> >          */
> >         __nested_copy_vmcb_save_to_cache(&save_cached, save);
> > @@ -1951,6 +1958,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> >         if (ret)
> >                 goto out_free;
> >
> > +       if (is_guest_mode(vcpu) && nested_npt_enabled(svm) &&
> > +           (kvm_state.hdr.svm.flags & KVM_STATE_SVM_VALID_GPAT))
> > +               svm->vmcb->save.g_pat = save_cached.g_pat;
> > +
> >         svm->nested.force_msr_bitmap_recalc = true;
> >
> >         kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
> > --
> > 2.52.0.457.g6b5491de43-goog
> >