Re: [PATCH 34/60] kvm: Keep track of plane VCPUs in struct kvm_vcpu_common

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Lack of reference counting in `kvm_vcpu_common_destroy` leads to a Use-After-Free write on the shared `common` structure.
- [High] The `vcpu_for_each_plane` macro contains an unclosed `if` statement, creating a severe dangling `else` vulnerability.
- [High] Missing memory barrier before publishing the VCPU pointer leads to lockless data races and read of uninitialized data.
- [Low] Missing parentheses around the `common` parameter in the `vcpu_for_each_plane` macro.
--

commit f70085341f8e117bed07bbc3da5525ab9b2b6aa4
Author: Joerg Roedel <joerg.roedel@amd.com>

Introduce an array which keeps track of per-plane VCPU instances for a
single VCPU index.

This commit adds an array to struct kvm_vcpu_common to track per-plane
VCPU instances and introduces a macro to iterate over them on plane
switches.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h

[ ... ]

> @@ -382,6 +384,10 @@ struct kvm_vcpu_common {
>  	struct kvm_vcpu_arch_common arch;
>  };
>  
> +#define vcpu_for_each_plane(common, i, v)			\
> +	for ((i) = 0; (i) < KVM_MAX_PLANES; ++(i))		\
> +		if (((v) = common->vcpus[(i)]) != NULL)

[Severity: High]
If a caller uses this macro inside an unbraced if block followed by an
else branch, will the C parser attach the else block to the macro's
internal if condition instead of the intended outer if? Could this
cause the else branch to execute for every NULL plane during iteration?

[Severity: Low]
Should the common argument be parenthesized as (common)->vcpus[(i)] to
ensure correct operator precedence if a caller passes a complex
expression to the macro?

[Severity: High]
Since the vcpu pointer is published without an smp_store_release() and
read here using plain loads instead of READ_ONCE(), could a concurrent
reader observe the VCPU pointer before its initialization is complete?

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> @@ -524,6 +524,8 @@ static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned
>  
>  static void kvm_vcpu_finish_common(struct kvm_vcpu *vcpu)
>  {
> +	WARN_ON(vcpu->common->vcpus[vcpu->plane_level] != NULL);
> +	vcpu->common->vcpus[vcpu->plane_level] = vcpu;
>  	smp_wmb();

[Severity: High]
Because the vcpu pointer is published to the shared array before the
smp_wmb() executes, could the CPU reorder the initialization stores to
happen after the pointer is visible? Would an smp_wmb() before the
assignment (or an smp_store_release) be needed to ensure concurrent
readers don't see uninitialized data?

>  	if (vcpu->plane_level == 0) {

[ ... ]

> @@ -555,6 +557,7 @@ static void kvm_vcpu_common_destroy(struct kvm_vcpu *vcpu)
>  
>  	vcpu->common = NULL;
>  	vcpu->run = NULL;
> +	common->vcpus[vcpu->plane_level] = NULL;
>  
>  	if (vcpu->plane_level != 0)
>  	       return;

[Severity: Critical]
If plane 0 is destroyed before secondary planes (for instance, during VM
teardown or an error path), will the shared common structure be freed
via kfree(common) when plane_level is 0? 

If a secondary plane is destroyed later, won't it write to the freed
memory when executing common->vcpus[vcpu->plane_level] = NULL? Does the
shared structure need a reference count to manage its lifetime safely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=34