Re: [PATCH 24/24] KVM: x86/mmu: use kvm_page_format to test SPTEs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The `spte_permission_fault()` function indexes the statically-sized 16-element `mmu->fmt.permissions` array using the raw hardware page fault error code (`fault->error_code`), leading to an out-of-bounds kernel memory read and host kernel panic when higher-order error code bits are set by hardware (e.g., AMD RMP faults).
--

commit 18ca0883f7313fb2ea6fc90d61d35c33ed81ed22
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Jun 3 10:58:14 2026 +0000

    KVM: x86/mmu: use kvm_page_format to test SPTEs
    
This commit replaces the custom is_access_allowed() implementation with 
spte_permission_fault(). The new version leverages kvm_page_format to check
SPTEs against permissions, adding support for SMEP and paving the way for
TDP entries with differing XS and XU bits.

> diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> index 918533e61b983..9bddfa0e02b97 100644
> --- a/arch/x86/kvm/mmu/spte.h
> +++ b/arch/x86/kvm/mmu/spte.h

[ ... ]

> @@ -496,20 +485,35 @@ static inline bool is_mmu_writable_spte(u64 spte)
>  }
>  
>  /*
> - * Returns true if the access indicated by @fault is allowed by the existing
> - * SPTE protections.  Note, the caller is responsible for checking that the
> - * SPTE is a shadow-present, leaf SPTE (either before or after).
> + * Returns true if the access indicated by @fault is forbidden by the existing
> + * SPTE protections.
>   */
> -static inline bool is_access_allowed(struct kvm_page_fault *fault, u64 spte)
> -{
> -	if (fault->exec)
> -		return is_executable_pte(spte);
> -
> -	if (fault->write)
> -		return is_writable_pte(spte);
> -
> -	/* Fault was on Read access */
> -	return spte & PT_PRESENT_MASK;
> +static inline bool spte_permission_fault(struct kvm_mmu *mmu, u64 spte,
> +					 struct kvm_page_fault *fault)
> +{
> +	unsigned int pfec = fault->error_code;
> +	int index = pfec >> 1;
> +	int pte_access;
> +
> +	if (!is_shadow_present_pte(spte))
> +		return true;
> +
> +	BUILD_BUG_ON(PT_PRESENT_MASK != ACC_READ_MASK);
> +	BUILD_BUG_ON(PT_WRITABLE_MASK != ACC_WRITE_MASK);
> +	BUILD_BUG_ON(VMX_EPT_READABLE_MASK != ACC_READ_MASK);
> +	BUILD_BUG_ON(VMX_EPT_WRITABLE_MASK != ACC_WRITE_MASK);
> +
> +	/* strip nested paging fault error codes */
> +	pte_access = spte & (PT_PRESENT_MASK | PT_WRITABLE_MASK);
> +	if (shadow_nx_mask) {
> +		pte_access |= spte & shadow_user_mask ? ACC_USER_MASK : 0;
> +		pte_access |= spte & shadow_nx_mask ? 0 : ACC_EXEC_MASK;
> +	} else {
> +		pte_access |= spte & shadow_xs_mask ? ACC_EXEC_MASK : 0;
> +		pte_access |= spte & shadow_xu_mask ? ACC_USER_EXEC_MASK : 0;
> +	}
> +
> +	return (mmu->fmt.permissions[index] >> pte_access) & 1;

[Severity: Critical]
Could this lead to an out-of-bounds read if the hardware page fault error
code contains higher-order bits?

Looking at spte_permission_fault(), the raw fault->error_code is shifted
and used directly as an index into the mmu->fmt.permissions array. Since
this array only has 16 elements, an unmasked error code with higher bits
set could easily exceed the array bounds.

For example, if an AMD RMP fault sets bit 31 (PFERR_GUEST_RMP_MASK), the
index would be 1073741824, potentially causing a host kernel panic when
accessing mmu->fmt.permissions[index]. Even without SEV-SNP, a PKRU fault
(bit 5) under shadow paging results in an index of 16, which is also out
of bounds.

Should fault->error_code be masked before computing the index?

>  }
>  
>  /*

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