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

On 2026/08/04 06:27 PM, [email protected] wrote:
> 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()?

These are the same concerns raised on patch 1/4 — please see the
response there. In summary: the pre-check is necessary for
stack-allocated structs, and the size field update follows the
documented versioning contract for extensible structs.

> 
> 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