Re: [PATCH v2 21/28] KVM: x86/mmu: Take memory protection attributes into account during faults

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] kvm: mmu: prefetching completely bypasses memory protections
- [Critical] kvm: mmu: missing SPTE invalidation allows bypass of shared memory protections
- [High] kvm: mmu: host access permissions conflated with guest root_role.access
--

commit 5aa6405feba846a8b81344070caa806c21c16bfd
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>
Subject: KVM: x86/mmu: Take memory protection attributes into account during faults

This commit updates KVM x86 MMU to respect memory protection attributes when
faulting guest memory. Prohibited accesses will cause a user-space -EFAULT
exit. The access type is checked using the existing kvm_page_format
permissions table.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 797b18201d982..3fd4cc3c57a53 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4642,6 +4642,50 @@ static int kvm_mmu_faultin_pfn_gmem(struct kvm_vcpu *vcpu,
>  	return RET_PF_CONTINUE;
>  }
>  
> +static inline unsigned kvm_get_gfn_protections(struct kvm_vcpu *vcpu, gfn_t gfn)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	unsigned int access = vcpu->arch.mmu->root_role.access;
> +	unsigned long attrs = kvm_get_memory_attributes(kvm, gfn);
> +	if (!attrs)
> +		return access;

[Severity: High]
Does initializing host access rules with the guest's root_role.access conflate
host and guest permissions, potentially causing spurious -EFAULT exits?

If attrs is 0, this returns the guest mask. Then in
kvm_faultin_memory_protections(), this access mask is evaluated against the
hardware fault code using __permission_fault().

If the guest's root page table is restricted (e.g., lacks write access), any
guest write fault could return -EFAULT, causing a KVM_EXIT_MEMORY_FAULT to
userspace instead of injecting a normal guest #PF. A guest could trigger this
by executing code with restricted root page table permissions.

> +
> +	WARN_ON_ONCE(!kvm_mem_attributes_valid(kvm, attrs));
> +
> +	if (!kvm_mem_attributes_may_read(attrs))
> +		access &= ~ACC_READ_MASK;
> +	if (!kvm_mem_attributes_may_write(attrs))
> +		access &= ~ACC_WRITE_MASK;
> +	if (!kvm_mem_attributes_may_exec(attrs)) {
> +		access &= ~ACC_EXEC_MASK;
> +		if (shadow_xu_mask)
> +			access &= ~ACC_USER_EXEC_MASK;
> +	}
> +
> +	return access;
> +}
> +
> +static int kvm_faultin_memory_protections(struct kvm_vcpu *vcpu,
> +					  struct kvm_page_fault *fault)
> +{
> +	unsigned access;
> +
> +	/* Memory attributes don't apply to MMIO regions */
> +	if (unlikely(!fault->slot))
> +		return RET_PF_CONTINUE;
> +
> +	access = kvm_get_gfn_protections(vcpu, fault->gfn);
> +	if (access == ACC_ALL)
> +		return RET_PF_CONTINUE;
> +
> +	trace_kvm_faultin_memory_protections(vcpu, fault, access);
> +	if (__permission_fault(vcpu->arch.mmu, access, fault))
> +		return -EFAULT;
> +
> +	fault->host_access &= access;
> +	return RET_PF_CONTINUE;
> +}

[Severity: Critical]
When memory protections are applied to shared memory, do we properly invalidate
existing writable SPTEs so that guests cannot bypass the protections?

Looking at kvm_arch_pre_set_memory_attributes() in arch/x86/kvm/mmu/mmu.c:

    /* Unmap the old attribute page. */
    if (range->arg.attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)
        range->attr_filter = KVM_FILTER_SHARED;
    else
        range->attr_filter = KVM_FILTER_PRIVATE;

    return kvm_unmap_gfn_range(kvm, range);

If userspace sets memory protections (like KVM_MEMORY_ATTRIBUTE_NW) without
setting PRIVATE, the filter falls back to KVM_FILTER_PRIVATE. The subsequent
call to kvm_unmap_gfn_range() only zaps private KVM_MIRROR_ROOTS and completely
skips shared KVM_DIRECT_ROOTS.

This could allow a guest to retain full read/write/execute access via existing
shared SPTEs to memory that was explicitly restricted by userspace.

[ ... ]

> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index e6ec14165f400..b031123fcb369 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -992,7 +992,7 @@ static int FNAME(sync_spte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, int
>  
>  	sptep = &sp->spt[i];
>  	spte = *sptep;
> -	host_access = ACC_ALL;
> +	host_access = kvm_get_gfn_protections(vcpu, gfn);

[Severity: Critical]
Since we are replacing ACC_ALL with kvm_get_gfn_protections() here, does
mmu_set_spte() also need to be updated to prevent prefetching from bypassing
memory protections?

Looking at mmu_set_spte() in arch/x86/kvm/mmu/mmu.c:

    unsigned host_access = fault ? fault->host_access : ACC_ALL;

If a guest touches a page adjacent to a protected page, triggering
kvm_mmu_prefetch_sptes(), mmu_set_spte() is called with a NULL fault pointer.
This causes it to default to ACC_ALL instead of querying the host attributes,
which could allow a malicious guest to bypass NR/NW/NX protections.

(Note: Patch 24 in this series attempts to fix this but introduces an inverted
condition "if (!kvm_get_memory_attributes(kvm, gfn)) continue;", which skips
prefetching only when there are NO attributes, and still grants ACC_ALL to
pages WITH attributes.)

>  	if (!(spte & shadow_host_writable_mask))
>  		host_access &= ~ACC_WRITE_MASK;
>  	slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);

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