Re: [PATCH v2 10/12] KVM: arm64: CCA: Prevent memslot modifications after Realm creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] KVM: arm64: non-Realm VMs bypass guest_memfd mmap and private
  memory checks
- [Low] KVM: arm64: stale memslot invalidation claim and missing macro
  in commit log
--

commit 72b617a00596c14c4599f4a8b9b980bcb6c02644
Author: Suzuki K Poulose <suzuki.poulose@arm.com>

> KVM: arm64: CCA: Prevent memslot modifications after Realm creation
>
> Once the Realm is created, prevent any changes to guest_memfd backed
> memslots, as the private memory is provided by slots backed by gmem.
> This will also be validated when we handle a fault for Realm Stage2 (in
> a later commit).
>
> Require Realm memslots to lie in the lower half of the IPA space, as the
> upper, shared half aliases the lower, protected half.
>
> A rejected deletion can already have invalidated the memslot before
> the prepare hook rejects it. Keep private mappings intact in this
> case.
>
> Additionally, similar to pKVM, we do not support dirty logging or
> read-only memslots.
>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>

[Severity: Low]
Is the paragraph about a rejected deletion invalidating the memslot
before the prepare hook rejects it left over from an earlier version?
This patch rejects guest_memfd memslot changes in
kvm_arch_validate_memory_region(), which commit "KVM: Allow architectures
to reject memslot changes before invalidation" runs before
kvm_invalidate_memslot().

Also, the commit message does not mention defining
kvm_arch_has_private_mem(kvm) for Realm VMs.

> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d672d87ffe620..a82b18a3e4e5d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1612,6 +1612,8 @@ struct kvm *kvm_arch_alloc_vm(void);
>  
>  #define kvm_vm_hyp_is_distrusting(kvm)	((kvm)->arch.vm_flavor >= __VM_DISTRUSTING_HYP)
>  
> +#define kvm_arch_has_private_mem(kvm) kvm_vm_is_realm(kvm)

[Severity: Medium]
Does defining kvm_arch_has_private_mem(kvm) as kvm_vm_is_realm(kvm) cause
non-Realm VMs to bypass guest_memfd mmap and private memory checks?

Because kvm_arch_has_private_mem is defined, include/linux/kvm_host.h
sets gmem_in_place_conversion to true for all arm64 VMs, so
kvm_gmem_prepare_memory_region() sets KVM_MEMSLOT_GMEM_ONLY on every
guest_memfd memslot even without GUEST_MEMFD_FLAG_MMAP.

However, commit "KVM: arm64: guest_memfd: Check mmap support when
preparing memslot" gated the !kvm_gmem_slot_supports_mmap(new) check in
kvm_arch_prepare_memory_region() on kvm_arch_has_private_mem(kvm), and
commit "KVM: arm64: CCA: Support runtime faulting of memory" likewise
gates the kvm_gmem_is_private_gfn() check in gmem_abort() on
kvm_arch_has_private_mem(kvm). As a result, a non-Realm VM can bind a
guest_memfd created without GUEST_MEMFD_FLAG_MMAP or
GUEST_MEMFD_FLAG_INIT_SHARED and fault pages marked
KVM_MEMORY_ATTRIBUTE_PRIVATE into its shared stage-2 page tables.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111145758.217946-1-steven.price@arm.com?part=10