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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional rejection of larger structs breaks the forward compatibility mechanism of copy_struct_from_user().
- [High] Overwriting the userspace size field with the kernel's struct size creates dangerous ambiguities on success.
--

--- Patch [4]: [PATCH v6 4/4] KVM: PPC: Document KVM_PPC_GET_COMPAT_CAPS ioctl ---
commit 5d4e2edcf1688edc6d21e5f2463ef8b58bd3d0a7
Author: Amit Machhiwal <[email protected]>

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 documentation covers error code descriptions including
E2BIG for forward compatibility and the extensible size-based versioning
contract.

> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index e3003a241d5b0..22fedb0aa34b4 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 (in/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

[Severity: High]
Does explicitly rejecting a larger struct size with -E2BIG break the standard
forward compatibility mechanism of copy_struct_from_user()?

Extensible struct UAPIs are designed to allow newer userspace to run seamlessly
on older kernels as long as new fields are zeroed. 

Looking at arch/powerpc/kvm/powerpc.c in kvm_arch_vm_ioctl(), the code
short-circuits the native trailing-zero check in copy_struct_from_user():

		if (usize > sizeof(host_caps)) {
			host_caps.size = sizeof(host_caps);
			r = -EFAULT;
			if (put_user(host_caps.size, (__u64 __user *)argp))
				goto out;
			r = -E2BIG;
			goto out;
		}

This forces userspace to implement fallback retry loops instead of relying on
the kernel's native UAPI extensibility.

[ ... ]

> +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. If userspace passes a larger
> +struct (``size > sizeof(struct kvm_ppc_compat_caps)``), the kernel writes
> +back its own struct size into the ``size`` field and returns ``-E2BIG``,
> +allowing userspace to discover the kernel's struct size and retry.

[Severity: High]
Can overwriting the userspace size field with the kernel's struct size create
dangerous ambiguities on success?

If userspace passes a smaller struct (e.g., 24 bytes) and the kernel uses a
larger struct (e.g., 32 bytes), the kernel updates host_caps.size to 32 and
copies 24 bytes back to userspace.

Looking at arch/powerpc/kvm/powerpc.c in kvm_arch_vm_ioctl():

		host_caps.size = sizeof(host_caps);
		r = copy_struct_to_user(argp, usize, &host_caps,
					sizeof(host_caps), NULL);

Userspace would receive a success code (0) but its size field is now 32. This
falsely indicates to userspace that the kernel provided 32 bytes of valid data,
which can lead to out-of-bounds reads if userspace trusts the returned size
field to determine valid data bounds within its buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4