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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The documentation specifies the ioctl returns `-ENOTTY` on non-pseries platforms, but the code returns `-EINVAL`.
- [Low] The ioctl parameter is documented as an `(out)` parameter, but it is actually an `(in/out)` parameter.

Pre-existing issues:
- [High] The UAPI unconditionally overwrites the user's `size` field with the kernel's struct size on success, leading to future user-space buffer overflows.
--

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

KVM: PPC: Document KVM_PPC_GET_COMPAT_CAPS ioctl

Add documentation for the KVM_PPC_GET_COMPAT_CAPS ioctl to the KVM API
documentation. The ioctl exposes host processor compatibility modes supported
for nested KVM guests on PowerPC systems.

> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index a5f9ee92f43e8..43810c451317f 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6566,6 +6566,85 @@ 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: Low]
Since userspace must set the size and flags fields before calling
the ioctl, should this parameter be designated as (in/out) rather than
just (out)?

> +: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
> +           ``KVM_PPC_COMPAT_CAPS_SIZE_VER0``, if the ``flags`` field
> +           is non-zero, or if the backend fails to retrieve or map
> +           CPU compatibility capabilities
> +  E2BIG    if ``size`` is larger than the kernel's struct size
> +           (new userspace on old kernel); the kernel writes back its
> +           own struct size into the ``size`` field so userspace can
> +           retry with the correct size
> +  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]
Is this error code accurate for non-pseries platforms?

Looking at kvmppc_get_compat_caps() in arch/powerpc/kvm/book3s_hv.c,
it appears to return -EINVAL instead:

arch/powerpc/kvm/book3s_hv.c:kvmppc_get_compat_caps() {
    long rc = -EINVAL;

    if (kvmhv_on_pseries()) {
        ...
    }

    if (rc < 0)
        return rc;
}

> +  ======== ============================================================
> +
> +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
> +KVM on PowerNV (nested API v1) and KVM on PowerVM (nested API v2) platforms.
> +
> +::
> +
> +  struct kvm_ppc_compat_caps {
> +	__u64	size;			/* Size of this structure */
> +	__u64	flags;			/* Reserved for future use, must be 0 */
> +	__u64	compat_capabilities;	/* Capabilities supported by the host */
> +  };
> +
> +Before calling this ioctl, userspace must set the ``size`` field to
> +``sizeof(struct kvm_ppc_compat_caps)`` and zero the ``flags`` field.
> +The kernel rejects non-zero ``flags`` with ``-EINVAL`` to prevent
> +uninitialized stack values from being silently accepted, keeping the
> +field available for future use without ABI ambiguity.
> +
> +The ioctl uses ``copy_struct_from_user()`` and ``copy_struct_to_user()``
> +to support extensible versioning: if userspace passes a struct smaller
> +than the current kernel version (``size >= KVM_PPC_COMPAT_CAPS_SIZE_VER0``),
> +the kernel zero-pads unknown trailing fields.

[Severity: High]
This isn't a bug introduced by this patch, but while documenting the extensible
versioning, does the existing code in kvm_arch_vm_ioctl() handle successful
calls correctly for smaller user structs?

arch/powerpc/kvm/powerpc.c:kvm_arch_vm_ioctl() {
    ...
        host_caps.size = sizeof(host_caps);
        r = copy_struct_to_user(argp, usize, &host_caps,
                                sizeof(host_caps), NULL);
    ...
}

By unconditionally overwriting host_caps.size with the kernel's struct size
on success, if the kernel struct is extended in the future (e.g., from 24 to
32 bytes), a userspace application passing size = 24 will get a success return
but have its size field overwritten with 32.

If userspace then reuses this struct for a subsequent ioctl call without
reinitializing the size field, it will pass size = 32. Will the kernel then read
and write 32 bytes to the user's 24-byte buffer, potentially corrupting memory?

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