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

[Amit Machhiwal <amachhiw@linux.ibm.com>]

On 2026/08/07 10:05 AM, Ritesh Harjani wrote:
> Amit Machhiwal <amachhiw@linux.ibm.com> writes:
> 
> > 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. The documentation covers error
> > code descriptions including E2BIG for forward compatibility, the
> > extensible size-based versioning contract using
> > KVM_PPC_COMPAT_CAPS_SIZE_VER0, the rationale for rejecting non-zero
> > reserved fields to prevent ABI ambiguity, bit numbering clarification
> > for IBM MSB-0 convention, and KVM-specific capability bit constants.
> >
> > Tested-by: Gautam Menghani <gautam@linux.ibm.com>
> > Reviewed-by: Gautam Menghani <gautam@linux.ibm.com>
> > Tested-by: Anushree Mathur <anushree.mathur@linux.ibm.com>
> > Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
> > ---
> >  Documentation/virt/kvm/api.rst | 79 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> >
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > index e3003a241d5b..22fedb0aa34b 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
> > +  ENOTTY   if the backend does not implement the ``get_compat_caps``
> > +           operation (e.g., on non-HV KVM implementations where the
> > +           required KVM operations are not available)
> 
> Amit, this may not be true anymore right after your changes in v7?
> Can we please update the documentation accordingly as well.

Agreed. After dropping the manual pre-check in patch-1 and delegating to
copy_struct_from_user(), -E2BIG is no longer unconditional when usize >
ksize — it only fires if the unknown trailing bytes are non-zero. Will
update the E2BIG entry to:

  E2BIG    if ``size`` is larger than the kernel's struct size and
           the unknown trailing bytes are non-zero (new userspace on
           old kernel with non-default fields set); the kernel writes
           back its own struct size into the ``size`` field so
           userspace can retry with the correct size

> 
> > +  ======== ============================================================
> > +
> > +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. If userspace passes a larger
> 
> So I already requested that we should fix this. We cannot write more
> bytes than requested by the user, since that memory may not be allocated
> for this struct in userspace.
> 
> On checking Sashiko comments in reply to this patch - I think that is
> also complaining of the same thing that it could cause buffer overflow.

copy_struct_to_user() itself is safe — it caps its write to min(ksize,
usize) bytes so it never writes past the user's buffer. However, the
problem is in the value written back in the size field: if usize <
sizeof(host_caps) (old userspace, new kernel), we'd write size =
sizeof(host_caps) into the first 8 bytes of the user's smaller buffer.
If userspace then reuses the struct naively, it would pass usize =
sizeof(host_caps) against its smaller allocation, which would cause an
actual overflow on the next call.

The fix is to write back usize instead:

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

This tells userspace "I filled exactly as many bytes as you gave me",
which is the correct contract for copy_struct_to_user().

Will update both the code in patch-1 and the versioning paragraph in the
documentation accordingly in v8.

Thanks,
Amit

> 
> 
> > +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.
> > +``KVM_PPC_COMPAT_CAPS_SIZE_VER0`` (24) is a frozen constant marking the
> > +size of the initial struct version.
> 
> Once we update the comments in patch-1 - I think we should correct this
> documentation too accordingly. We should just simply use
> copy_to|from_user_struct() style for doing this.
> 
> > +
> > +The ``compat_capabilities`` bit field describes the processor compatibility
> > +modes supported by the host. The following bits indicate support for specific
> > +processor modes (using IBM's MSB-0 convention where bit 0 is the most
> > +significant bit):
> > +
> > +- ``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
> > +
> > +.. note::
> > +
> > +   The bit numbering above uses IBM's MSB-0 convention (bit 0 is the most
> > +   significant bit). In the actual implementation, these are defined as:
> > +
> > +   - ``KVM_PPC_COMPAT_CAP_POWER9``  = ``(1ULL << 62)``
> > +   - ``KVM_PPC_COMPAT_CAP_POWER10`` = ``(1ULL << 61)``
> > +   - ``KVM_PPC_COMPAT_CAP_POWER11`` = ``(1ULL << 60)``
> > +
> > +   Userspace should use the defined constants from ``<linux/kvm.h>`` rather
> > +   than hardcoding bit positions.
> > +
> >  .. _kvm_run:
> >  
> >  5. The kvm_run structure
> > -- 
> > 2.50.1 (Apple Git-155)