Re: [PATCH v2 22/28] KVM: x86/mmu: Issue memory fault exit if walk failed due to memory attribute

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: KVM_EXIT_MEMORY_FAULT reports original access type instead of PTE read
- [High] kvm: memory attribute violations during emulation inject spurious #PF
--

commit f13a0662fba26c69b7ab24d7639e44c8da7ee24c
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: x86/mmu: Issue memory fault exit if walk failed due to memory attribute

This patch adds a check during guest page table walks to identify if a walk
fails due to a memory attribute restriction. When this occurs, it populates
the newly added memory_attributes_fault flag and attempts to exit to userspace
with a memory fault.

> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index b031123fcb369..d2836a327bc99 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
[ ... ]
> @@ -411,6 +413,12 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
>  		if (unlikely(kvm_is_error_hva(host_addr)))
>  			goto error;
>  
> +		if (!kvm_mem_attributes_may_read_gfn(vcpu->kvm, gpa_to_gfn(real_gpa))) {
> +			walker->memory_attributes_fault = true;
> +			walker->gfn = gpa_to_gfn(real_gpa);
> +			goto error;
> +		}
> +

[Severity: High]
When this flag is set and FNAME(walk_addr_generic)() returns an error, how is
it handled by other callers that aren't updated in this patch?

For example, in arch/x86/kvm/mmu/paging_tmpl.h within FNAME(gva_to_gpa)():

static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, struct kvm_pagewalk *w,
			       gpa_t addr, u64 access,
			       struct x86_exception *exception)
{
	struct guest_walker walker;
	gpa_t gpa = INVALID_GPA;
	int r;

	r = FNAME(walk_addr_generic)(&walker, vcpu, w, addr, access);

	if (r) {
		gpa = gfn_to_gpa(walker.gfn);
		gpa |= addr & ~PAGE_MASK;
	} else if (exception)
		*exception = walker.fault;

If instruction emulation encounters a memory attribute violation here during a
page table walk, does this drop the walker.memory_attributes_fault flag and
blindly return the fabricated #PF exception?

Could this inject a spurious #PF into the guest instead of exiting to userspace
with KVM_EXIT_MEMORY_FAULT?

[ ... ]
> @@ -820,6 +828,12 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>  	 * The page is not mapped by the guest.  Let the guest handle it.
>  	 */
>  	if (!r) {
> +		if (walker.memory_attributes_fault) {
> +			fault->gfn = walker.gfn;
> +			kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
> +			return -EFAULT;
> +		}
> +

[Severity: High]
In arch/x86/kvm/mmu/paging_tmpl.h within FNAME(page_fault)(), if
walker.memory_attributes_fault is true, we prepare a memory fault exit.

However, does kvm_mmu_prepare_memory_fault_exit() use the original fault's
properties (fault->write, fault->exec) instead of the properties of the page
table read itself?

Could this incorrectly report the access flags to userspace (e.g., reporting
WRITE or EXEC instead of READ) when a page table walk fails due to KVM memory
attributes?

>  		if (!fault->prefetch)
>  			__kvm_inject_emulated_page_fault(vcpu, &walker.fault, true);
>

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