Re: [PATCH] KVM: nVMX: Prevent vmlaunch with EPTP pointing outside assigned memory area

[Yan Zhao]

On Wed, Jun 28, 2023 at 08:37:45AM -0700, Sean Christopherson wrote:
...
> So I think we should try this:
> 
> ---
>  arch/x86/kvm/mmu/mmu.c   | 19 -------------------
>  include/linux/kvm_host.h |  1 -
>  virt/kvm/kvm_main.c      | 13 ++-----------
>  3 files changed, 2 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 60397a1beda3..e305737edf84 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3671,19 +3671,6 @@ void kvm_mmu_free_guest_mode_roots(struct kvm *kvm, struct kvm_mmu *mmu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_mmu_free_guest_mode_roots);
>  
> -
> -static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn)
> -{
> -	int ret = 0;
> -
> -	if (!kvm_vcpu_is_visible_gfn(vcpu, root_gfn)) {
> -		kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> -		ret = 1;
> -	}
> -
> -	return ret;
> -}
> -
>  static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, int quadrant,
>  			    u8 level)
>  {
> @@ -3821,9 +3808,6 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
>  	root_pgd = kvm_mmu_get_guest_pgd(vcpu, mmu);
>  	root_gfn = root_pgd >> PAGE_SHIFT;
>  
> -	if (mmu_check_root(vcpu, root_gfn))
> -		return 1;
> -
Hi Sean,
The checking of existence of memslot is still useful,
Otherwise NULL pointer dereference would be met as in below call trace.

mmu_alloc_shadow_roots
 |->mmu_alloc_root
    |->mmu_alloc_root(root_gfn)
       |->kvm_mmu_get_shadow_page
          |->__kvm_mmu_get_shadow_page
             |->kvm_mmu_alloc_shadow_page
                |->account_shadowed
                   |->slot = __gfn_to_memslot(slots, gfn);                    ==>NULL pointer
                   |  kvm_slot_page_track_add_page(kvm, slot, gfn,KVM_PAGE_TRACK_WRITE);
                      |->update_gfn_track(slot, gfn, mode, 1);
                         |->index = gfn_to_index(gfn, slot->base_gfn, PG_LEVEL_4K);  ==>NULL pointer dereference


Thanks
Yan

>  	/*
>  	 * On SVM, reading PDPTRs might access guest memory, which might fault
>  	 * and thus might sleep.  Grab the PDPTRs before acquiring mmu_lock.
> @@ -3833,9 +3817,6 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
>  			pdptrs[i] = mmu->get_pdptr(vcpu, i);
>  			if (!(pdptrs[i] & PT_PRESENT_MASK))
>  				continue;
> -
> -			if (mmu_check_root(vcpu, pdptrs[i] >> PAGE_SHIFT))
> -				return 1;
>  		}
>  	}
>