| 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
| ||
|
Message-ID: <aYRBE1tICOiQ/RL0@yzhao56-desk.sh.intel.com>
Date: Thu, 5 Feb 2026 15:04:51 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>, Thomas Gleixner
<tglx@...nel.org>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
<bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
Kiryl Shutsemau <kas@...nel.org>, Paolo Bonzini <pbonzini@...hat.com>,
<linux-kernel@...r.kernel.org>, <linux-coco@...ts.linux.dev>,
<kvm@...r.kernel.org>, Kai Huang <kai.huang@...el.com>, Rick Edgecombe
<rick.p.edgecombe@...el.com>, Vishal Annapurve <vannapurve@...gle.com>,
Ackerley Tng <ackerleytng@...gle.com>, Sagi Shahar <sagis@...gle.com>,
"Binbin Wu" <binbin.wu@...ux.intel.com>, Xiaoyao Li <xiaoyao.li@...el.com>,
"Isaku Yamahata" <isaku.yamahata@...el.com>
Subject: Re: [RFC PATCH v5 09/45] KVM: x86: Rework .free_external_spt() into
.reclaim_external_sp()
On Wed, Feb 04, 2026 at 05:45:39PM +0800, Yan Zhao wrote:
> On Wed, Jan 28, 2026 at 05:14:41PM -0800, Sean Christopherson wrote:
> > Massage .free_external_spt() into .reclaim_external_sp() to free up (pun
> > intended) "free" for actually freeing memory, and to allow TDX to do more
> > than just "free" the S-EPT entry. Specifically, nullify external_spt to
> > leak the S-EPT page if reclaiming the page fails, as that detail and
> > implementation choice has no business living in the TDP MMU.
> >
> > Use "sp" instead of "spt" even though "spt" is arguably more accurate, as
> > "spte" and "spt" are dangerously close in name, and because the key
> > parameter is a kvm_mmu_page, not a pointer to an S-EPT page table.
> >
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > ---
> > arch/x86/include/asm/kvm-x86-ops.h | 2 +-
> > arch/x86/include/asm/kvm_host.h | 4 ++--
> > arch/x86/kvm/mmu/tdp_mmu.c | 13 ++-----------
> > arch/x86/kvm/vmx/tdx.c | 27 ++++++++++++---------------
> > 4 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> > index 57eb1f4832ae..c17cedc485c9 100644
> > --- a/arch/x86/include/asm/kvm-x86-ops.h
> > +++ b/arch/x86/include/asm/kvm-x86-ops.h
> > @@ -95,8 +95,8 @@ KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
> > KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
> > KVM_X86_OP(load_mmu_pgd)
> > KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
> > -KVM_X86_OP_OPTIONAL_RET0(free_external_spt)
> > KVM_X86_OP_OPTIONAL(remove_external_spte)
> > +KVM_X86_OP_OPTIONAL(reclaim_external_sp)
> > KVM_X86_OP(has_wbinvd_exit)
> > KVM_X86_OP(get_l2_tsc_offset)
> > KVM_X86_OP(get_l2_tsc_multiplier)
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index d12ca0f8a348..b35a07ed11fb 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1858,8 +1858,8 @@ struct kvm_x86_ops {
> > u64 mirror_spte);
> >
> > /* Update external page tables for page table about to be freed. */
> > - int (*free_external_spt)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> > - void *external_spt);
> > + void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
> > + struct kvm_mmu_page *sp);
> Do you think "free" is still better than "reclaim" though TDX actually
> invokes tdx_reclaim_page() to reclaim it on the TDX side?
>
> Naming it free_external_sp can be interpreted as freeing the sp->external_spt
> externally (vs freeing it in tdp_mmu_free_sp_rcu_callback(). This naming also
> allows for the future possibility of freeing sp->external_spt before the HKID is
> freed (though this is unlikely).
Oh. I found there's a free_external_sp() in patch 20.
So, maybe reclaim_external_sp() --> remove_external_spt() ?
Still think "sp" is not good :)
> > /* Update external page table from spte getting removed, and flush TLB. */
> > void (*remove_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 27ac520f2a89..18764dbc97ea 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -456,17 +456,8 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> > old_spte, FROZEN_SPTE, level, shared);
> > }
> >
> > - if (is_mirror_sp(sp) &&
> > - WARN_ON(kvm_x86_call(free_external_spt)(kvm, base_gfn, sp->role.level,
> > - sp->external_spt))) {
> > - /*
> > - * Failed to free page table page in mirror page table and
> > - * there is nothing to do further.
> > - * Intentionally leak the page to prevent the kernel from
> > - * accessing the encrypted page.
> > - */
> > - sp->external_spt = NULL;
> > - }
> > + if (is_mirror_sp(sp))
> > + kvm_x86_call(reclaim_external_sp)(kvm, base_gfn, sp);
> >
> > call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> > }
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 30494f9ceb31..66bc3ceb5e17 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1783,27 +1783,24 @@ static void tdx_track(struct kvm *kvm)
> > kvm_make_all_cpus_request(kvm, KVM_REQ_OUTSIDE_GUEST_MODE);
> > }
> >
> > -static int tdx_sept_free_private_spt(struct kvm *kvm, gfn_t gfn,
> > - enum pg_level level, void *private_spt)
> > +static void tdx_sept_reclaim_private_sp(struct kvm *kvm, gfn_t gfn,
> > + struct kvm_mmu_page *sp)
> Passing in "sp" and having "reclaim_private_sp" in the function name is bit
> confusing.
> Strictly speaking, only sp->external_spt is private, while the sp and sp->spt
> are just mirroring the external spt.
>
> But I understand it's for setting sp->external_spt to NULL on error.
>
> > {
> > - struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > -
> > /*
> > - * free_external_spt() is only called after hkid is freed when TD is
> > - * tearing down.
> > * KVM doesn't (yet) zap page table pages in mirror page table while
> > * TD is active, though guest pages mapped in mirror page table could be
> > * zapped during TD is active, e.g. for shared <-> private conversion
> > * and slot move/deletion.
> > + *
> > + * In other words, KVM should only free mirror page tables after the
> > + * TD's hkid is freed, when the TD is being torn down.
> > + *
> > + * If the S-EPT PTE can't be removed for any reason, intentionally leak
> > + * the page to prevent the kernel from accessing the encrypted page.
> > */
> > - if (KVM_BUG_ON(is_hkid_assigned(kvm_tdx), kvm))
> > - return -EIO;
> > -
> > - /*
> > - * The HKID assigned to this TD was already freed and cache was
> > - * already flushed. We don't have to flush again.
> > - */
> > - return tdx_reclaim_page(virt_to_page(private_spt));
> > + if (KVM_BUG_ON(is_hkid_assigned(to_kvm_tdx(kvm)), kvm) ||
> > + tdx_reclaim_page(virt_to_page(sp->external_spt)))
> > + sp->external_spt = NULL;
> > }
> >
> > static void tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
> > @@ -3617,7 +3614,7 @@ void __init tdx_hardware_setup(void)
> > vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size, sizeof(struct kvm_tdx));
> >
> > vt_x86_ops.set_external_spte = tdx_sept_set_private_spte;
> > - vt_x86_ops.free_external_spt = tdx_sept_free_private_spt;
> > + vt_x86_ops.reclaim_external_sp = tdx_sept_reclaim_private_sp;
> > vt_x86_ops.remove_external_spte = tdx_sept_remove_private_spte;
> > vt_x86_ops.protected_apic_has_interrupt = tdx_protected_apic_has_interrupt;
> > }
> > --
> > 2.53.0.rc1.217.geba53bf80e-goog
> >
Powered by blists - more mailing lists