Re: [PATCH v4 2/4] KVM: PPC: Book3S HV: Implement compat CPU capability retrieval for KVM on PowerVM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Strict size validation of the extensible `struct kvm_ppc_compat_caps` prevents future extensions without breaking backward compatibility.
- [High] Raw hypervisor `H_*` error codes are directly leaked to userspace as POSIX error numbers, causing incorrect system call mapping and potential infinite loops.
- [High] The ioctl incorrectly queries raw L0 hypervisor capabilities instead of the capabilities actually negotiated by the L1 KVM host.
- [Medium] Device tree node reference leak in `for_each_node_by_type` loop.
--

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

KVM: PPC: Book3S HV: Implement compat CPU capability retrieval for KVM on PowerVM

On POWER systems, the host CPU may run in a compatibility mode (e.g., a
Power11 processor operating in Power10 compatibility mode). In such
cases, the effective CPU level exposed to guests differs from the
physical processor generation.

[ ... ]

> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> index 8a38be6c3b03b..7304886814435 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -443,7 +443,16 @@ struct kvm_ppc_compat_caps {
>  	__u64	size;			/* Size of this structure */
>  	__u64	compat_capabilities;	/* Capabilities supported by the host */
>  };

[Severity: High]
The KVM_PPC_GET_COMPAT_CAPS ioctl is defined using
_IOR(KVMIO, 0xe4, struct kvm_ppc_compat_caps). Since _IOR embeds the
structure size in the ioctl number, if fields are added to this
structure in the future, the ioctl number will change.

Additionally, the explicit size validation in kvm_arch_vm_ioctl() rejects
requests where user_size < sizeof(host_caps), returning -EINVAL:

arch/powerpc/kvm/powerpc.c:kvm_arch_vm_ioctl() {
    ...
		/* Validate size - must be at least the current structure size */
		r = -EINVAL;
		if (user_size < sizeof(host_caps))
			goto out;
    ...
}

Does this prevent extending the structure in the future without breaking
backward UAPI compatibility for older userspace binaries?

> +/*
> + * Capability bits for compat_capabilities field in kvm_ppc_compat_caps.
> + * These bits indicate which processor compatibility modes are supported.
> + */
> +#define KVM_PPC_COMPAT_CAP_POWER9	(1ULL << 62)

[ ... ]

> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 61dbeea317f3b..5ef6713343bed 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -6510,6 +6510,22 @@ static bool kvmppc_hash_v3_possible(void)

[ ... ]

> +static int kvmppc_get_compat_caps(struct kvm_ppc_compat_caps *host_caps)
> +{
> +	unsigned long capabilities = 0;
> +	long rc = -EINVAL;
> +
> +	if (kvmhv_on_pseries()) {
> +		if (kvmhv_is_nestedv2())
> +			rc = plpar_guest_get_capabilities(0, &capabilities);

[Severity: High]
Are we returning a raw hypervisor error code directly to userspace here?

Negative codes like H_PARAMETER (-4) could be incorrectly interpreted as
standard POSIX errors (e.g., -EINTR). Standard userspace libraries might
handle EINTR by automatically retrying the ioctl, which could lead to an
infinite loop.

Positive codes like H_NOT_AVAILABLE (3) will be returned directly to the
userspace caller, skipping the copy_to_user logic in kvm_arch_vm_ioctl()
while still appearing as a success to userspace.

Should we map these raw hypervisor status codes to standard POSIX errors
before returning?

[Severity: High]
Does this query the raw L0 hypervisor capabilities instead of the capabilities
actively negotiated by the L1 KVM host?

According to the PAPR nested virtualization specification, this hypercall
retrieves unmodified L0 capabilities. However, KVM limits its nested guest
capabilities based on nested_capabilities negotiated during initialization:

arch/powerpc/kvm/book3s_hv_nested.c:kvmhv_nestedv2_init() {
    ...
		nested_capabilities = capabilities & host_capabilities;
		rc = plpar_guest_set_capabilities(0, nested_capabilities);
    ...
}

If QEMU receives raw L0 capabilities, could it attempt to use CPU modes that
KVM will subsequently reject during vCPU initialization, leading to guest
boot failures?

Should we return the negotiated nested_capabilities instead?

> +		host_caps->compat_capabilities = capabilities &
> +							KVM_PPC_COMPAT_BITMASK;
> +	}
> +
> +	return rc;
> +}

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