Re: [PATCH 12/24] KVM: x86/mmu: move CPU-related fields to struct kvm_pagewalk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] When using nested virtualization with shadow paging (EPT disabled), L1's PDPTRs are overwritten by L2's PDPTRs during a nested VM-Entry but are never restored on a failed VM-Entry. This allows L2 to control L1's shadow page tables.
--

commit a87d952b84013700c29cd0401e35d07fb7710cf4
Author: Paolo Bonzini <pbonzini@redhat.com>

    KVM: x86/mmu: move CPU-related fields to struct kvm_pagewalk

This commit moves CPU-related fields and their page format dependencies
into struct kvm_pagewalk. This allows the walk_mmu function to remain
self-contained while cleaning up the accessor functions.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4fbb7508e2413..e2bfecf655d9a 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c

[ ... ]

> @@ -4083,7 +4083,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
>  	 * On SVM, reading PDPTRs might access guest memory, which might fault
>  	 * and thus might sleep.  Grab the PDPTRs before acquiring mmu_lock.
>  	 */
> -	if (mmu->cpu_role.base.level == PT32E_ROOT_LEVEL) {
> +	if (mmu->w.cpu_role.base.level == PT32E_ROOT_LEVEL) {
>  		for (i = 0; i < 4; ++i) {
>  			pdptrs[i] = mmu->w.get_pdptr(vcpu, i);

[Severity: Critical]
This is a pre-existing issue, but while reviewing the PDPTR accesses here,
I noticed a problem with how PDPTRs are handled on nested VM-Entry failures.

When using nested virtualization with shadow paging (EPT disabled), L1's
PDPTRs are overwritten by L2's PDPTRs during a nested VM-Entry but are never
restored on a failed VM-Entry.

Looking at nested_vmx_restore_host_state() in arch/x86/kvm/vmx/nested.c:

	vcpu->arch.cr3 = vmcs_readl(GUEST_CR3);
	kvm_register_mark_available(vcpu, VCPU_REG_CR3);

	/*
	 * Use ept_save_pdptrs(vcpu) to load the MMU's cached PDPTRs
	 * from vmcs01 (if necessary).  The PDPTRs are not loaded on
	 * VMFail, like everything else we just need to ensure our
	 * software model is up-to-date.
	 */
	if (enable_ept && is_pae_paging(vcpu))
		ept_save_pdptrs(vcpu);

L1's CR3 is restored here upon a VM-Entry failure, but the PDPTRs are only
restored if EPT is enabled. If shadow paging is used (enable_ept is false),
the L2 PDPTRs loaded during the aborted entry attempt remain in KVM's software
model (vcpu->arch.pdptrs).

Furthermore, in vmx_cache_reg() in arch/x86/kvm/vmx/vmx.c:

	case VCPU_REG_PDPTR:
		if (enable_ept)
			ept_save_pdptrs(vcpu);
		break;

After the failed VM-Entry, VCPU_REG_PDPTR is marked as unavailable. When KVM's
shadow MMU reads the PDPTRs via get_pdptr(), it calls vmx_cache_reg(). For
shadow paging, this does nothing and simply marks the register as available,
causing the MMU to use the stale L2 PDPTRs.

Since L2 can provide arbitrary addresses for its PDPTRs, does this enable L2
to hijack L1's memory mappings and escalate privileges from the nested guest
to the L1 hypervisor?

>  			if (!(pdptrs[i] & PT_PRESENT_MASK))
>  				continue;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603105814.10236-1-pbonzini@redhat.com?part=12