Re: [PATCH] KVM: arm64: vgic: Annotate struct vgic_dist with __counted_by_ptr

[Kees Cook <kees@kernel.org>]

On Thu, Mar 19, 2026 at 01:54:10AM +0000, Bill Wendling wrote:
> Add the __counted_by_ptr attribute to the spis pointer field in struct
> vgic_dist. This pointer field points to an array of struct vgic_irq
> elements, and the number of elements is tracked by the nr_spis field
> within the same structure.
> 
> The nr_spis field is initialized in vgic_init() (or earlier via
> userspace) before the spis array is allocated in kvm_vgic_dist_init().
> The nr_spis value remains constant during the lifetime of the spis
> allocation, making it a suitable counter for the array.

This one eluded me briefly, but yeah:

int vgic_init(struct kvm *kvm)
{
        struct vgic_dist *dist = &kvm->arch.vgic;
	...
        /* freeze the number of spis */
        if (!dist->nr_spis)
                dist->nr_spis = VGIC_NR_IRQS_LEGACY - VGIC_NR_PRIVATE_IRQS;

        ret = kvm_vgic_dist_init(kvm, dist->nr_spis);



static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
{
        struct vgic_dist *dist = &kvm->arch.vgic;
	...
        dist->spis = kzalloc_objs(struct vgic_irq, nr_spis, GFP_KERNEL_ACCOUNT);


It seems weird that nr_spis is passed to kvm_vgic_dist_init at all:
there's only 1 caller, and the value is stored in dist->nr_spis
(available through kvm) immediately before the call. For readability,
I'd almost prefer to see:


diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 9b3091ad868c..9e6ca2f04581 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -190,16 +190,15 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 /**
  * kvm_vgic_dist_init: initialize the dist data structures
  * @kvm: kvm struct pointer
- * @nr_spis: number of spis, frozen by caller
  */
-static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
+static int kvm_vgic_dist_init(struct kvm *kvm)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);
 	int i;
 
 	dist->active_spis = (atomic_t)ATOMIC_INIT(0);
-	dist->spis = kzalloc_objs(struct vgic_irq, nr_spis, GFP_KERNEL_ACCOUNT);
+	dist->spis = kzalloc_objs(struct vgic_irq, dist->nr_spis, GFP_KERNEL_ACCOUNT);
 	if (!dist->spis)
 		return  -ENOMEM;
 
@@ -211,7 +210,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
 	 * require prior initialization in case of a virtual GICv3 or trigger
 	 * initialization when using a virtual GICv2.
 	 */
-	for (i = 0; i < nr_spis; i++) {
+	for (i = 0; i < dist->nr_spis; i++) {
 		struct vgic_irq *irq = &dist->spis[i];
 
 		irq->intid = i + VGIC_NR_PRIVATE_IRQS;
@@ -401,7 +400,7 @@ int vgic_init(struct kvm *kvm)
 	if (!dist->nr_spis)
 		dist->nr_spis = VGIC_NR_IRQS_LEGACY - VGIC_NR_PRIVATE_IRQS;
 
-	ret = kvm_vgic_dist_init(kvm, dist->nr_spis);
+	ret = kvm_vgic_dist_init(kvm);
 	if (ret)
 		goto out;
 


The other question I'd have (which isn't related to this patch
specifically) is if dist->spis is guaranteed to be NULL before
kvm_vgic_dist_init() is called.

Regardless:

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

> 
> This patch was generated by CodeMender and reviewed by Bill Wendling.
> Tested with the KVM selftests.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oupton@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Zenghui Yu <yuzenghui@huawei.com>
> Cc: Gogul Balakrishnan <bgogul@google.com>
> Cc: Arman Hasanzadeh <armanihm@google.com>
> Cc: Kees Cook <kees@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.linux.dev
> Cc: linux-kernel@vger.kernel.org
> Cc: codemender-patching+linux@google.com
> ---
>  include/kvm/arm_vgic.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f2eafc65bbf4..1cca87623d92 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -284,7 +284,7 @@ struct vgic_dist {
>  	/* Wants SGIs without active state */
>  	bool			nassgireq;
>  
> -	struct vgic_irq		*spis;
> +	struct vgic_irq		*spis __counted_by_ptr(nr_spis);
>  
>  	struct vgic_io_device	dist_iodev;
>  	struct vgic_io_device	cpuif_iodev;
> -- 
> 2.53.0.851.ga537e3e6e9-goog
> 

-- 
Kees Cook