Re: [PATCH v4 4/4] KVM: PPC: Document KVM_PPC_GET_COMPAT_CAPS ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] UAPI backwards compatibility breakage due to invalid struct size validation.
- [High] Reserved `flags` field is not checked for zero, rendering it permanently unusable for future input.
- [High] Capability mapping logic lacks `fallthrough`, artificially restricting nested guests.
- [High] Resource leak of `device_node` refcount in `kvmppc_get_compat_caps()`.
- [Medium] Misleading documentation causing userspace to pass uninitialized memory to the kernel.
- [Medium] UAPI misconfiguration: IOCTL direction is `_IOR` but kernel reads from userspace.
- [Medium] Inconsistent error return code on non-pseries platforms contradicts documentation.
--

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

KVM: PPC: Document KVM_PPC_GET_COMPAT_CAPS ioctl

This commit adds documentation for the KVM_PPC_GET_COMPAT_CAPS ioctl in the
KVM API. It details the capabilities, structure fields, and expected error
codes.

> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 52bbbb553ce10..ba6feba74d7d9 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6553,6 +6553,53 @@ KVM_S390_KEYOP_SSKE
>    Sets the storage key for the guest address ``guest_addr`` to the key
>    specified in ``key``, returning the previous value in ``key``.
>  
> +4.145 KVM_PPC_GET_COMPAT_CAPS
> +-----------------------------
> +:Capability: KVM_CAP_PPC_COMPAT_CAPS
> +:Architectures: powerpc
> +:Type: vm ioctl
> +:Parameters: struct kvm_ppc_compat_caps (out)

[Severity: Medium]
Is this parameter accurately described as strictly output-only?

Looking at the existing implementation, the kernel reads the size field from
userspace:

arch/powerpc/kvm/powerpc.c:kvm_arch_vm_ioctl() {
...
        if (copy_from_user(&user_size, &((struct kvm_ppc_compat_caps
             __user *)argp)->size, sizeof(user_size))) {
...
}

If userspace leaves the struct uninitialized based on this documentation, the
kernel will read random memory for size, leading to -EINVAL failures. Should
the documentation indicate this is an input/output parameter?


[Severity: Medium]
This isn't a bug introduced by this patch, but should the ioctl direction be
updated?

The IOCTL is currently defined as read-only:

include/uapi/linux/kvm.h:
#define KVM_PPC_GET_COMPAT_CAPS _IOR(KVMIO,  0xe4, struct kvm_ppc_compat_caps)

Since the kernel reads the size field from userspace, this violates
conventions and can cause issues with tracing tools. Should it be defined
with _IOWR?

> +:Returns: 0 on success, negative value on failure
> +
> +Errors include:
> +
> +  ======== ============================================================
> +  EFAULT   if ``struct kvm_ppc_compat_caps`` cannot be read from or
> +           written to userspace
> +  EINVAL   if the ``size`` field is smaller than the current structure
> +           size, or if the backend implementation fails to retrieve or
> +           map CPU compatibility capabilities

[Severity: High]
This isn't a bug introduced by this patch, but could the current size
validation break backwards compatibility?

The existing implementation in arch/powerpc/kvm/powerpc.c rejects requests if
the size is smaller than the host structure:

arch/powerpc/kvm/powerpc.c:kvm_arch_vm_ioctl() {
...
    if (user_size < sizeof(host_caps))
        goto out;
...
}

If struct kvm_ppc_compat_caps is ever extended, older userspace binaries
passing the old, smaller structure size will be unconditionally rejected.

> +  ENOTTY   if the backend does not implement the ``get_compat_caps``
> +           operation (e.g., on non-pseries platforms or when the
> +           required KVM operations are not available)
> +  ======== ============================================================

[Severity: Medium]
Will the existing implementation return -ENOTTY as documented here?

Looking at kvmppc_get_compat_caps(), it returns -EINVAL on non-pseries
platforms:

arch/powerpc/kvm/book3s_hv.c:kvmppc_get_compat_caps() {
    int rc = -EINVAL;
    ...
    if (kvmhv_on_pseries()) { ... }
    return rc;
}

This could confuse userspace into thinking it passed an invalid argument size.
Should the documentation or the code be adjusted?

> +
> +IBM POWER system server-based processors provide a compatibility mode feature
> +where an Nth generation processor can operate in modes consistent with earlier
> +generations such as (N-1) and (N-2).
> +
> +This ioctl provides userspace with information about the CPU compatibility modes
> +supported by the current host processor for booting the nested KVM guests on
> +PowerNV (KVM nested APIv1) and PowerVM (KVM nested APIv2) platforms.
> +
> +::
> +
> +  struct kvm_ppc_compat_caps {
> +	__u64	flags;			/* Reserved for future use */

[Severity: High]
This isn't a bug introduced by this patch, but is the reserved flags field
permanently unusable for future input?

The existing implementation skips copying and validating the flags field:

arch/powerpc/kvm/powerpc.c:kvm_arch_vm_ioctl() {
...
    if (copy_from_user(&user_size, &((struct kvm_ppc_compat_caps
         __user *)argp)->size, sizeof(user_size))) {
...
}

Because the parameter is documented as (out) and the kernel ignores any
uninitialized data in flags, a future kernel will not be able to safely
repurpose flags for input. Should this validate that flags is zero?

> +	__u64	size;			/* Size of this structure */
> +	__u64	compat_capabilities;	/* Capabilities supported by the host */
> +  };
> +
> +The ``compat_capabilities`` bit field describes the processor compatibility
> +modes supported by the host. For example, the following bits indicate support
> +for specific processor modes.
> +
> +::
> +
> +  KVM_PPC_COMPAT_CAP_POWER9  (bit 1): KVM guests can run in Power9 processor mode
> +  KVM_PPC_COMPAT_CAP_POWER10 (bit 2): KVM guests can run in Power10 processor mode
> +  KVM_PPC_COMPAT_CAP_POWER11 (bit 3): KVM guests can run in Power11 processor mode

[Severity: High]
This isn't a bug introduced by this patch, but can the capability mapping
logic falsely restrict nested guests?

The switch statement lacks fallthrough annotations:

arch/powerpc/kvm/book3s_hv.c:kvmppc_map_compat_capabilities() {
    ...
    case PVR_ARCH_31_P11:
        *capabilities |= KVM_PPC_COMPAT_CAP_POWER11;
        break;
    ...
}

This causes the host to report a capability mask with only a single bit,
indicating it cannot run nested guests in older compatibility modes.


[Severity: High]
This is a pre-existing issue, but are we missing a reference count cleanup
here?

In the compatibility capabilities retrieval:

arch/powerpc/kvm/book3s_hv.c:kvmppc_get_compat_caps() {
    ...
    for_each_node_by_type(np, "cpu") {
        prop = of_get_property(np, "cpu-version", NULL);
        if (prop) {
            cpu_version = be32_to_cpup(prop);
            break;
        }
    }
    ...
}

The for_each_node_by_type macro takes a reference count on the device node.
Exiting the loop early via break requires manually dropping the reference via
of_node_put(np). Does this leak a reference on every ioctl call?

>  .. _kvm_run:
>  
>  5. The kvm_run structure

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