Re: [PATCH v4 3/4] KVM: PPC: Book3S HV: Add support for compat CPU capabilities for KVM on PowerNV

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing `of_node_put()` causes an OF node reference count leak when breaking out of `for_each_node_by_type()`.
- [High] Incomplete capability reporting due to missing `fallthrough` in `kvmppc_map_compat_capabilities`.
- [High] Hypercall H_ error codes are leaked directly to userspace as standard Linux errnos.
- [Medium] KVM_PPC_GET_COMPAT_CAPS is defined as _IOR but expects input from userspace.
- [Low] Endianness type mismatch for `cpu_version` between caller and callee violates sparse typing.
--

commit 2d0f9bcccc58cf2776556c592aad6db1adf0c8aa
Author: Amit Machhiwal <amachhiw@linux.ibm.com>

    KVM: PPC: Book3S HV: Add support for compat CPU capabilities for KVM on PowerNV

    Extend host CPU compatibility capability reporting to support nested
    virtualization on PowerNV systems. For nested API v1, derive the
    capabilities from the L1 guest via the "cpu-version" device tree property
    and map them to KVM_PPC_COMPAT_CAP bits.

> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 61dbeea317f3b..dd0f4d487a770 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
[ ... ]
> @@ -6510,6 +6510,57 @@ static bool kvmppc_hash_v3_possible(void)
>  	return true;
>  }
>  
> +static int kvmppc_map_compat_capabilities(const __be32 cpu_version,
> +				      unsigned long *capabilities)

[Severity: Low]
Does this parameter type violate sparse typing rules?

The caller kvmppc_get_compat_caps() passes a host-endian u32 value that was
converted via be32_to_cpup(). If cpu_version is annotated as __be32, might
this trigger a static analyzer warning?

> +{
> +	switch (cpu_version) {
> +	case PVR_ARCH_31_P11:
> +		*capabilities |= KVM_PPC_COMPAT_CAP_POWER11;
> +		break;
> +	case PVR_ARCH_31:
> +		*capabilities |= KVM_PPC_COMPAT_CAP_POWER10;
> +		break;
> +	case PVR_ARCH_300:
> +		*capabilities |= KVM_PPC_COMPAT_CAP_POWER9;
> +		break;

[Severity: High]
Could this switch statement be missing fallthroughs?

Host CPU compatibility modes are typically cumulative. For example, a POWER10
CPU would support both POWER10 and POWER9 compatibility modes. With these
breaks, only a single highest capability bit is set. Is there a risk QEMU
might incorrectly conclude that older nested guests are unsupported?

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int kvmppc_get_compat_caps(struct kvm_ppc_compat_caps *host_caps)
> +{
> +	struct device_node *np;
> +	unsigned long capabilities = 0;
> +	const __be32 *prop = NULL;
> +	long rc = -EINVAL;
> +	u32 cpu_version;
> +
> +	if (kvmhv_on_pseries()) {
> +		if (kvmhv_is_nestedv2()) {
> +			rc = plpar_guest_get_capabilities(0, &capabilities);

[Severity: High]
Are hypercall error codes being leaked directly to userspace here?

If plpar_guest_get_capabilities() fails, it returns a hypervisor error code
like H_PARAMETER (-4). This value is directly assigned to rc and returned to
the ioctl handler. Since -4 corresponds to -EINTR in Linux, would userspace
misinterpret the error as an interrupted system call and infinitely retry the
ioctl?

> +		} else {
> +			for_each_node_by_type(np, "cpu") {
> +				prop = of_get_property(np, "cpu-version", NULL);
> +				if (prop) {
> +					cpu_version = be32_to_cpup(prop);
> +					break;

[Severity: High]
Does this early break leak the OF node reference?

The for_each_node_by_type() macro drops the reference to the previous node
and acquires a reference to the current node. By breaking out of the loop
without calling of_node_put(np), the reference count appears to be leaked.

> +				}
> +			}
> +			if (!prop)
> +				return -EINVAL;
> +			rc = kvmppc_map_compat_capabilities(cpu_version,
> +								&capabilities);
> +		}
> +		host_caps->compat_capabilities = capabilities &
> +							KVM_PPC_COMPAT_BITMASK;
> +	}
> +
> +	return rc;
> +}
[ ... ]
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6c8afa2047bf3..1788a00686626 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
[ ... ]
> @@ -1349,6 +1350,9 @@ struct kvm_s390_keyop {
>  #define KVM_GET_DEVICE_ATTR	  _IOW(KVMIO,  0xe2, struct kvm_device_attr)
>  #define KVM_HAS_DEVICE_ATTR	  _IOW(KVMIO,  0xe3, struct kvm_device_attr)
>  
> +/* Available with KVM_CAP_PPC_COMPAT_CAPS */
> +#define KVM_PPC_GET_COMPAT_CAPS	_IOR(KVMIO,  0xe4, struct kvm_ppc_compat_caps)

[Severity: Medium]
Could this ioctl definition cause issues with wrapper libraries?

The KVM_PPC_GET_COMPAT_CAPS ioctl handler in kvm_arch_vm_ioctl() uses
copy_from_user() to read the size field from the userspace structure.
Since it reads input data from userspace, should this be defined as _IOWR?
Using _IOR might cause unexpected behavior with tools that assume the
parameter is purely an output buffer.

> +
>  /*
>   * ioctls for vcpu fds
>   */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616123314.82721-1-amachhiw@linux.ibm.com?part=3