Re: [PATCH 09/24] KVM: Introduce NR/NW/NX memory attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] KVM advertises and accepts memory protection attributes (NR/NW/NX) but fails to enforce them in the MMU, leading to guest sandbox bypass.
- [High] A guest-controlled ioctl can trigger an unbounded, non-yielding loop holding `mmu_lock` in write mode, causing host soft lockups.
- [Medium] `kvm_range_has_memory_attributes()` incorrectly assumes any present XArray entry means non-zero attributes, ignoring the `mask`.
- [Low] Incorrect usage of `WARN_ON_ONCE` with a string literal condition.
- [Low] Leftover debugging code (`trace_printk`) and unresolved `// todo:` comments in production code.
--

--- Patch [9]: [PATCH 09/24] KVM: Introduce NR/NW/NX memory attributes ---
commit dce195fe8dd20af5f86a55891fc7d52cd1a8ed11
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: Introduce NR/NW/NX memory attributes

This commit introduces memory attributes to map guest physical memory
regions as non-readable, non-writable, and/or non-executable. It defines
the KVM_MEMORY_ATTRIBUTE_NR/NW/NX flags and adds basic architecture-specific
callbacks for x86 to support these protection attributes.

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 3b235e9539239..6a96eb3218dc4 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1653,6 +1653,9 @@ struct kvm_memory_attributes {
>  	__u64 flags;
>  };
>  
> +#define KVM_MEMORY_ATTRIBUTE_NR		       (1ULL << 0)
> +#define KVM_MEMORY_ATTRIBUTE_NW		       (1ULL << 1)
> +#define KVM_MEMORY_ATTRIBUTE_NX		       (1ULL << 2)

[Severity: Medium]
Does the introduction of multiple memory attributes break the fast-path
optimization in kvm_range_has_memory_attributes()?

If attrs == 0, the fast-path optimization ignores the mask:

virt/kvm/kvm_main.c:kvm_range_has_memory_attributes() {
    ...
	if (!attrs)
		return !xas_find(&xas, end - 1);
    ...
}

Now that there are multiple attributes, a page might have
KVM_MEMORY_ATTRIBUTE_NR set but KVM_MEMORY_ATTRIBUTE_PRIVATE cleared.
If a caller checks if PRIVATE is 0 (i.e. mask = KVM_MEMORY_ATTRIBUTE_PRIVATE,
attrs = 0), the function incorrectly returns false simply because the NR entry
exists in the XArray.

>  #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
>  
>  #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)

[ ... ]

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 80f581afb6a01..b6463b0b0b6d4 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -8088,16 +8087,23 @@ bool kvm_arch_pre_set_memory_attributes(struct kvm *kvm,
>  	}
>  
>  	/*
> -	 * Zap SPTEs even if the slot can't be mapped PRIVATE.  KVM x86 only
> -	 * supports KVM_MEMORY_ATTRIBUTE_PRIVATE, and so it *seems* like KVM
> -	 * can simply ignore such slots.  But if userspace is making memory
> -	 * PRIVATE, then KVM must prevent the guest from accessing the memory
> -	 * as shared.  And if userspace is making memory SHARED and this point
> -	 * is reached, then at least one page within the range was previously
> -	 * PRIVATE, i.e. the slot's possible hugepage ranges are changing.
> -	 * Zapping SPTEs in this case ensures KVM will reassess whether or not
> -	 * a hugepage can be used for affected ranges.
> +	 * For KVM_MEMORY_ATTRIBUTE_PRIVATE:
> +	 *  Zap SPTEs even if the slot can't be mapped PRIVATE.  KVM x86 only
> +	 *  supports KVM_MEMORY_ATTRIBUTE_PRIVATE, and so it *seems* like KVM
> +	 *  can simply ignore such slots.  But if userspace is making memory
> +	 *  PRIVATE, then KVM must prevent the guest from accessing the memory
> +	 *  as shared.  And if userspace is making memory SHARED and this point
> +	 *  is reached, then at least one page within the range was previously
> +	 *  PRIVATE, i.e. the slot's possible hugepage ranges are changing.
> +	 *  Zapping SPTEs in this case ensures KVM will reassess whether or not
> +	 *  a hugepage can be used for affected ranges.
> +	 *
> +	 * For KVM_MEMORY_ATTRIBUTE_NR/NW/NX:
> +	 *  Zap even when loosening restrictions R=>RW, which is nost strictly
> +	 *  necessary, but will allow KVM to reasses whether a hugepage can be
> +	 *  used for the affected pages.
>  	 */

[Severity: High]
Does setting these new attributes trigger an unbounded, non-yielding loop
holding the MMU lock?

When the KVM_SET_MEMORY_ATTRIBUTES ioctl is called, it eventually calls
kvm_arch_post_set_memory_attributes():

arch/x86/kvm/mmu/mmu.c:kvm_arch_post_set_memory_attributes() {
    ...
	for (gfn_t gfn = range->start; gfn < range->end; gfn++) {
		for_each_gfn_valid_sp_with_gptes(kvm, sp, gfn) {
			gen_update = true;
			trace_printk("needs gen update! %llx\n", gfn);
			goto exit_loop;
		}
	}
exit_loop:
    ...
	// todo: what to do with kvm_tdp_mmu_zap_invalidated_roots()?
	// add kvm_arch_post_set_memory_attributes_unlocked?
	return false;
}

Since the GFN range is controlled by the ioctl and can be massive, and the
loop executes under mmu_lock for write without cond_resched(), could this
freeze KVM and trigger host RCU stalls and soft lockups if no GPTEs are
found to break the loop early?

[Severity: Low]
Are the trace_printk() and literal "// todo:" comments in the above snippet
intended to remain in production code?

Using trace_printk() permanently allocates a large trace ring buffer at boot
and emits a loud warning banner in dmesg.

> +
>  	if (WARN_ON_ONCE(range->end <= range->start))
>  		return false;

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index fa4473d7c920a..a47b62c2c9cee 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2421,10 +2421,15 @@ static int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm,
>  #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
>  u64 kvm_supported_mem_attributes(struct kvm *kvm)
>  {
> +	u64 supported_attrs = 0;
> +
> +	if (kvm_arch_has_memory_protection_attributes(kvm))
> +		supported_attrs |= KVM_MEMORY_ATTRIBUTE_PROT;
> +

[Severity: Critical]
Does KVM advertise and accept memory protection attributes without enforcing
them in the MMU?

KVM advertises KVM_MEMORY_ATTRIBUTE_PROT support to userspace here and allows
setting NR/NW/NX. However, the page fault handlers (e.g.,
__kvm_mmu_faultin_pfn, direct_page_fault, kvm_tdp_mmu_map) do not appear to
query kvm_get_memory_attributes() to intersect the requested access with the
memory attributes.

When the guest faults again after the SPTEs are zapped, the SPTE is rebuilt
with full host permissions, bypassing the memory protections entirely. Can this
allow a guest to escape the intended sandbox?

>  	if (kvm_arch_has_private_mem(kvm))
> -		return KVM_MEMORY_ATTRIBUTE_PRIVATE;
> +		supported_attrs |= KVM_MEMORY_ATTRIBUTE_PRIVATE;
>  
> -	return 0;
> +	return supported_attrs;
>  }

[ ... ]

> @@ -2596,6 +2601,25 @@ static int kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
>  
>  	return r;
>  }
> +
> +bool __weak kvm_arch_mem_attributes_supported_prot(struct kvm *kvm, unsigned long attrs)
> +{
> +	WARN_ON_ONCE("KVM_MEMORY_ATTRIBUTE_PROT requires kvm_arch_mem_attributes_supported_prot()");

[Severity: Low]
Should this use WARN_ONCE(1, "...") instead?

The WARN_ON_ONCE macro expects a boolean expression. Since a string literal
evaluates to true, this will print the string's memory address or pointer as
the condition in the stack trace instead of the intended message.

> +	return false;
> +}

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