Re: [PATCH v7 17/20] KVM: arm64: Add vCPU device attr to partition the PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM_ARM_VCPU_PMU_V3_ENABLE_PARTITION modifies VM-wide state without checking if the VM has already run, allowing state corruption.
- [High] get_user() reads a 32-bit unsigned int directly into a 1-byte bool, leading to truncation bugs and failing to enforce strict UAPI input validation.
- [Medium] kvm_arm_pmu_v3_has_attr() unconditionally advertises support for the KVM_ARM_VCPU_PMU_V3_ENABLE_PARTITION attribute even when the host hardware does not support it.
- [Low] Mismatched kernel-doc comments for kvm_vcpu_pmu_is_partitioned() and kvm_pmu_partition_enable().
--

commit d9172235a5a8d816682a8078c38e635415737a2b
Author: Colton Lewis <coltonlewis@google.com>

KVM: arm64: Add vCPU device attr to partition the PMU

This commit introduces a new PMU device attribute to enable partitioned
PMU support for a VM. The capability can be configured before the vCPU starts,
provided PMUv3 and VHE are supported and the host driver has reserved
counters.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 9693d9eb69daa..47fd143cf4ac7 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c
> @@ -44,8 +44,8 @@ bool kvm_pmu_is_partitioned(struct arm_pmu *pmu)
>  }
>  
>  /**
> - * kvm_vcpu_pmu_is_partitioned() - Determine if given VCPU has a partitioned PMU
> - * @vcpu: Pointer to kvm_vcpu struct
> + * kvm_pmu_is_partitioned() - Determine if given VCPU has a partitioned PMU
> + * @kvm: Pointer to kvm_vcpu struct
>   *
>   * Determine if given VCPU has a partitioned PMU by extracting that
>   * field and passing it to :c:func:`kvm_pmu_is_partitioned`

This isn't a bug, but the function name and parameter in the kernel-doc
block don't match the actual function signature below:

> @@ -55,7 +55,36 @@ bool kvm_pmu_is_partitioned(struct arm_pmu *pmu)
>  bool kvm_vcpu_pmu_is_partitioned(struct kvm_vcpu *vcpu)

[ ... ]

> +/**
> + * kvm_pmu_partition_enable() - Enable/disable partition flag
> + * @kvm: Pointer to vcpu
> + * @enable: Whether to enable or disable
> + *
> + * If we want to enable the partition, the guest is free to grab
> + * hardware by accessing PMU registers. Otherwise, the host maintains
> + * control.
> + */
> +void kvm_pmu_partition_enable(struct kvm *kvm, bool enable)

This isn't a bug, but the comment describes @kvm as a pointer to a vcpu,
while the function takes a pointer to struct kvm.

> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index f1c66ce678840..add5e7da830b2 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c
[ ... ]
> @@ -759,6 +759,19 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  
>  		return kvm_arm_pmu_v3_set_nr_counters(vcpu, n);
>  	}
> +	case KVM_ARM_VCPU_PMU_V3_ENABLE_PARTITION: {
> +		unsigned int __user *uaddr = (unsigned int __user *)(long)attr->addr;
> +		bool enable;
> +
> +		if (get_user(enable, uaddr))
> +			return -EFAULT;

Is it possible for the get_user() call to truncate the value being read?

Since uaddr is cast to unsigned int __user *, the inline assembly inside
get_user() will load a 32-bit value from userspace. However, the output
variable enable is a 1-byte bool. If userspace passes a value like 256
(0x100), the lowest byte is 0, which could incorrectly result in false.

Additionally, should we explicitly check that the input value is exactly
0 or 1? Accepting any arbitrary value here burns the upper 31 bits and
prevents repurposing them for future flags without breaking backward
compatibility.

> +
> +		if (!has_kvm_pmu_partition_support())
> +			return -EPERM;
> +
> +		kvm_pmu_partition_enable(kvm, enable);
> +		return 0;
> +	}

Could this sequence result in concurrent state modification?

In a multi-vCPU VM, if userspace initializes and starts running vCPU 0, it
could subsequently invoke this ioctl on an uninitialized vCPU 1. Since
kvm_arm_pmu_v3_set_attr() only checks if the individual vCPU is initialized:

	if (vcpu->arch.pmu.created)
		return -EBUSY;

and KVM_ARM_VCPU_PMU_V3_ENABLE_PARTITION modifies VM-wide state without
calling kvm_vm_has_ran_once(kvm), the PMU partition mode could be altered
while vCPU 0 is already running.

[ ... ]

> @@ -798,6 +811,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	case KVM_ARM_VCPU_PMU_V3_FILTER:
>  	case KVM_ARM_VCPU_PMU_V3_SET_PMU:
>  	case KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS:
> +	case KVM_ARM_VCPU_PMU_V3_ENABLE_PARTITION:
>  		if (kvm_vcpu_has_pmu(vcpu))
>  			return 0;
>  	}

Will this advertise the partition capability even when the host hardware
does not support it?

The has_attr handler returns 0 based solely on kvm_vcpu_has_pmu(vcpu), but
the set_attr handler requires has_kvm_pmu_partition_support() to be true. If
userspace relies on KVM_HAS_DEVICE_ATTR to probe capabilities, it might
incorrectly attempt to configure the feature and receive an unexpected -EPERM.
Should this return -ENXIO if has_kvm_pmu_partition_support() is false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504211813.1804997-1-coltonlewis@google.com?part=17