Re: [PATCH] KVM: arm64: Add kvm_for_each_vncr_tlb() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The `kvm_for_each_vncr_tlb` macro introduces an unbraced `if` statement, exposing callers to a dangling `else` vulnerability.

Pre-existing issues:
- [High] Concurrent lockless publication and observation of `vcpu->arch.vncr_tlb` lacks memory barriers and `READ_ONCE()`, risking uninitialized memory access.
- [High] Unconditional allocation in `kvm_vcpu_allocate_vncr_tlb` leaks memory if `kvm_arch_vcpu_run_pid_change` is restarted.
--

commit 03a7f368fcb61f7b51d0623ed1c15d57166b377e
Author: Marc Zyngier <maz@kernel.org>
Subject: KVM: arm64: Add kvm_for_each_vncr_tlb() helper

This commit introduces a new helper macro kvm_for_each_vncr_tlb() to 
iterate over vCPUs that have a valid vncr_tlb allocated. It then
converts kvm_invalidate_vncr_ipa() and invalidate_vncr_va() to use
this new iterator.

Link: https://lore.kernel.org/r/aiUvSbrWndQeUPc8@v4bel

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 883b6c1008fbb..e25d7289aa9e5 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -897,9 +897,21 @@ static void invalidate_vncr(struct vncr_tlb *vt)
>  		clear_fixmap(vncr_fixmap(vt->cpu));
>  }
>  
> +/*
> + * VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions, and
> + * either can race against a vcpu not being onlined yet (no pseudo-TLB
> + * allocated). Similarly, the TLB might be invalid.  Skip those, as they
> + * obviously don't participate in the invalidation at this stage.
> + */
> +#define kvm_for_each_vncr_tlb(idx, vcpup, tlbp, kvm)	\
> +	kvm_for_each_vcpu(idx, vcpup, kvm)		\
> +		if (((tlbp) = vcpup->arch.vncr_tlb) &&	\
> +		    (tlbp)->valid)

[Severity: High]
This isn't a bug introduced by this patch, but can we read uninitialized
memory here due to a missing memory barrier?

When vcpu->arch.vncr_tlb is allocated in kvm_vcpu_allocate_vncr_tlb(),
it is assigned via a plain store without smp_store_release(), and not
under kvm->mmu_lock.

Concurrently, the MMU notifier path (which holds kvm->mmu_lock) iterates
over vCPUs and reads this pointer locklessly using a plain load here without
READ_ONCE() or smp_load_acquire(). 

On weakly-ordered architectures, could the observing CPU see a non-NULL
pointer before the zero-initialization of kzalloc_obj() is visible?

If so, tlbp->valid could be evaluated using uninitialized memory.
If it evaluates to true, invalidate_vncr() could read garbage from
vt->cpu and trigger the BUG_ON() in vncr_fixmap(), causing a host panic.

[Severity: High]
This is a pre-existing issue, but does kvm_vcpu_allocate_vncr_tlb() leak
memory if KVM_RUN is restarted?

In kvm_arch_vcpu_run_pid_change(), kvm_vcpu_allocate_vncr_tlb() is
called during the first KVM_RUN and unconditionally allocates the pointer
with kzalloc_obj().

If subsequent initialization steps in kvm_arch_vcpu_run_pid_change() fail
(like kvm_timer_enable()), it returns an error to userspace without
marking the vCPU as having run.

If userspace repeatedly invokes the KVM_RUN ioctl after a failure,
could kvm_vcpu_allocate_vncr_tlb() be executed again, overwriting the
previous vncr_tlb allocation without freeing it and leaking memory?

[Severity: Low]
Does the unbraced if in this macro expose callers to a dangling else
vulnerability?

While current usages are safely enclosed in braces, if a future caller uses
this macro inside an unbraced if block and attempts to attach an else,
the compiler could incorrectly bind it to this hidden internal if.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607175745.297793-1-maz@kernel.org?part=1