Re: [PATCH 5/5] kvm: x86: mmu: Verify that restored PTE has needed perms in fast page fault

Paolo Bonzini
2017-01-25T11:49:10Z

> - remove_acc_track = is_access_track_spte(spte);
> -
> - /* Verify that the fault can be handled in the fast path */
> - if (!remove_acc_track && !remove_write_prot)
> - break;
> + new_spte = is_access_track_spte(spte)
> + ? restore_acc_track_spte(spte)
> + : spte;

Just a tiny stylistic change to match the "new_spte |= PT_WRITABLE_MASK"
in the block below:

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index aa3b0d14c019..2fd7586aad4d 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3135,9 +3135,10 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
break;
}

- new_spte = is_access_track_spte(spte)
- ? restore_acc_track_spte(spte)
- : spte;
+ new_spte = spte;
+
+ if (is_access_track_spte(spte))
+ new_spte = restore_acc_track_spte(new_spte);

/*
* Currently, to simplify the code, write-protection can

Paolo

> /*
> - * Do not fix write-permission on the large spte since we only
> - * dirty the first page into the dirty-bitmap in
> - * fast_pf_fix_direct_spte() that means other pages are missed
> - * if its slot is dirty-logged.
> - *
> - * Instead, we let the slow page fault path create a normal spte
> - * to fix the access.
> - *
> - * See the comments in kvm_arch_commit_memory_region().
> + * Currently, to simplify the code, write-protection can
> + * be removed in the fast path only if the SPTE was
> + * write-protected for dirty-logging or access tracking.
> */
> - if (sp->role.level > PT_PAGE_TABLE_LEVEL && remove_write_prot)
> + if ((error_code & PFERR_WRITE_MASK) &&
> + spte_can_locklessly_be_made_writable(spte))
> + {
> + new_spte |= PT_WRITABLE_MASK;
> +
> + /*
> + * Do not fix write-permission on the large spte since
> + * we only dirty the first page into the dirty-bitmap in
> + * fast_pf_fix_direct_spte() that means other pages are
> + * missed if its slot is dirty-logged.
> + *
> + * Instead, we let the slow page fault path create a
> + * normal spte to fix the access.
> + *
> + * See the comments in kvm_arch_commit_memory_region().
> + */
> + if (sp->role.level > PT_PAGE_TABLE_LEVEL)
> + break;
> + }
> +
> + /* Verify that the fault can be handled in the fast path */
> + if (new_spte == spte ||
> + !is_access_allowed(error_code, new_spte))
> break;
>
> /*
> @@ -3166,8 +3175,7 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
> */
> fault_handled = fast_pf_fix_direct_spte(vcpu, sp,
> iterator.sptep, spte,
> - remove_write_prot,
> - remove_acc_track);
> + new_spte);
> if (fault_handled)
> break;
>
>