On Fri, Feb 06, 2026 at 11:12:36AM -0800, Sean Christopherson wrote: > On Fri, Feb 06, 2026, Jim Mattson wrote: > > On Fri, Feb 6, 2026 at 10:23 AM Yosry Ahmed <[email protected]> wrote: > > > > > > February 6, 2026 at 10:19 AM, "Sean Christopherson" <[email protected]> > > > wrote: > > > > > > > > > > > > > > On Thu, Feb 05, 2026, Jim Mattson wrote: > > > > > > > > > > > > > > Cache g_pat from vmcb12 in svm->nested.gpat to avoid TOCTTOU issues, > > > > > and > > > > > add a validity check so that when nested paging is enabled for > > > > > vmcb12, an > > > > > invalid g_pat causes an immediate VMEXIT with exit code > > > > > VMEXIT_INVALID, as > > > > > specified in the APM, volume 2: "Nested Paging and VMRUN/VMEXIT." > > > > > > > > > > Fixes: 3d6368ef580a ("KVM: SVM: Add VMRUN handler") > > > > > Signed-off-by: Jim Mattson <[email protected]> > > > > > --- > > > > > arch/x86/kvm/svm/nested.c | 4 +++- > > > > > arch/x86/kvm/svm/svm.h | 3 +++ > > > > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > > > > > index f72dbd10dcad..1d4ff6408b34 100644 > > > > > --- a/arch/x86/kvm/svm/nested.c > > > > > +++ b/arch/x86/kvm/svm/nested.c > > > > > @@ -1027,9 +1027,11 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu) > > > > > > > > > > nested_copy_vmcb_control_to_cache(svm, &vmcb12->control); > > > > > nested_copy_vmcb_save_to_cache(svm, &vmcb12->save); > > > > > + svm->nested.gpat = vmcb12->save.g_pat; > > > > > > > > > > if (!nested_vmcb_check_save(vcpu) || > > > > > - !nested_vmcb_check_controls(vcpu)) { > > > > > + !nested_vmcb_check_controls(vcpu) || > > > > > + (nested_npt_enabled(svm) && !kvm_pat_valid(svm->nested.gpat))) { > > > > > vmcb12->control.exit_code = SVM_EXIT_ERR; > > > > > vmcb12->control.exit_info_1 = 0; > > > > > vmcb12->control.exit_info_2 = 0; > > > > > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h > > > > > index 986d90f2d4ca..42a4bf83b3aa 100644 > > > > > --- a/arch/x86/kvm/svm/svm.h > > > > > +++ b/arch/x86/kvm/svm/svm.h > > > > > @@ -208,6 +208,9 @@ struct svm_nested_state { > > > > > */ > > > > > struct vmcb_save_area_cached save; > > > > > > > > > > + /* Cached guest PAT from vmcb12.save.g_pat */ > > > > > + u64 gpat; > > > > > > > > > Shouldn't this go in vmcb_save_area_cached? > > > > > > I believe Jim changed it after this discussion on v2: > > > https://lore.kernel.org/kvm/[email protected]/. > > LOL, oh the irony: > > I'm going to cache it on its own to avoid confusion. > > > Right. The two issues with putting it in vmcb_save_area_cached were: > > > > 1. Checking all of vmcb_save_area_cached requires access to the > > corresponding control area (or at least the boolean, "NTP enabled.") > > Checking the control area seems like the right answer (I went down that path > before reading this). > > > 2. In the nested state serialization payload, everything else in the > > vmcb_save_area_cached comes from L1 (host state to be restored at > > emulated #VMEXIT.) > > Hmm, right, but *because* it's ignored, that gives us carte blanche to > clobber it. > More below. > > > The first issue was a little messy, but not that distasteful. > > I actually find it the opposite of distasteful. KVM definitely _should_ be > checking the controls, not the vCPU state. If it weren't for needing to get > at > MAXPHYADDR in CPUID, I'd push to drop @vcpu entirely. > > > The second issue was really a mess. > > I'd rather have the mess contained and document though. Caching g_pat outside > of vmcb_save_area_cached bleeds the mess into all of the relevant nSVM code, > and > doesn't leave any breadcrumbs in the code/comments to explain that it "needs" > to > be kept separate. > > AFAICT, the only "problem" is that g_pat in the serialization payload will be > garbage when restoring state from an older KVM. But that's totally fine, > precisely > because L1's PAT isn't restored from vmcb01 on nested #VMEXIT, it's always > resident > in vcpu->arch.pat. So can't we just do this to avoid a spurious -EINVAL? > > /* > * Validate host state saved from before VMRUN (see > * nested_svm_check_permissions). > */ > __nested_copy_vmcb_save_to_cache(&save_cached, save); > > /* > * Stuff gPAT in L1's save state, as older KVM may not have saved L1's > * gPAT. L1's PAT, i.e. hPAT for the vCPU, is *always* tracked in > * vcpu->arch.pat, i.e. gPAT is a reflection of vcpu->arch.pat, not the > * other way around. > */ > save_cached.g_pat = vcpu->arch.pat; > > if (!(save->cr0 & X86_CR0_PG) || > !(save->cr0 & X86_CR0_PE) || > (save->rflags & X86_EFLAGS_VM) || > !nested_vmcb_check_save(vcpu, &ctl_cached, &save_cached)) > goto out_free; > > Oh, and if we do plumb in @ctrl to __nested_vmcb_check_save(), I vote to > opportunistically drop the useless single-use wrappers (probably in a > standalone > patch to plumb in @ctrl). E.g. (completely untested)
They are dropped by https://lore.kernel.org/kvm/[email protected]/.
