linux-kernel - Re: [PATCH v6 15/18] KVM: x86/tdp_mmu: Propagate tearing down mirror page tables

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: <Z4r_XNcxPWpgjZio@google.com>
Date: Fri, 17 Jan 2025 17:09:48 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Paolo Bonzini <pbonzini@...hat.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org, yan.y.zhao@...el.com, 
	isaku.yamahata@...el.com, binbin.wu@...ux.intel.com, 
	rick.p.edgecombe@...el.com, Kai Huang <kai.huang@...el.com>
Subject: Re: [PATCH v6 15/18] KVM: x86/tdp_mmu: Propagate tearing down mirror
 page tables

On Sun, Dec 22, 2024, Paolo Bonzini wrote:
> +	/* Because write lock is held, operation should success. */

succeed.

> +	ret = static_call(kvm_x86_remove_external_spte)(kvm, gfn, level, old_pfn);
> +	KVM_BUG_ON(ret, kvm);
> +}
> +
>  /**
>   * handle_removed_pt() - handle a page table removed from the TDP structure
>   *
> @@ -435,6 +458,23 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
>  		}
>  		handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn,
>  				    old_spte, FROZEN_SPTE, level, shared);
> +
> +		if (is_mirror_sp(sp)) {
> +			KVM_BUG_ON(shared, kvm);

Should these bail early if the KVM_BUG_ON() is hit?  Calling into the TDX module
after bugging the VM is a bit odd.

> +			remove_external_spte(kvm, gfn, old_spte, level);
> +		}
> +	}
> +
> +	if (is_mirror_sp(sp) &&
> +	    WARN_ON(static_call(kvm_x86_free_external_spt)(kvm, base_gfn, sp->role.level,

WARN_ON_ONCE(). I suspect that if this ever gets hit, it'll come in bunches.

> +							  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;
>  	}
>  
>  	call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> @@ -608,6 +648,13 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
>  	if (is_mirror_sptep(iter->sptep) && !is_frozen_spte(new_spte)) {
>  		int ret;
>  
> +		/*
> +		 * Users of atomic zapping don't operate on mirror roots,
> +		 * so don't handle it and bug the VM if it's seen.
> +		 */
> +		if (KVM_BUG_ON(!is_shadow_present_pte(new_spte), kvm))
> +			return -EBUSY;
> +
>  		ret = set_external_spte_present(kvm, iter->sptep, iter->gfn,
>  						iter->old_spte, new_spte, iter->level);
>  		if (ret)
> @@ -700,8 +747,10 @@ static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
>  	 * Users that do non-atomic setting of PTEs don't operate on mirror
>  	 * roots, so don't handle it and bug the VM if it's seen.
>  	 */
> -	if (is_mirror_sptep(sptep))
> +	if (is_mirror_sptep(sptep)) {
>  		KVM_BUG_ON(is_shadow_present_pte(new_spte), kvm);
> +		remove_external_spte(kvm, gfn, old_spte, level);
> +	}
>  
>  	return old_spte;
>  }
> -- 
> 2.43.5
> 
>