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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The KVM_PPC_GET_COMPAT_CAPS ioctl overwrites the user-provided size field with the kernel's struct size on a successful call, which creates a buffer overflow vulnerability in user space if the struct is reused.
--

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

KVM: PPC: Document KVM_PPC_GET_COMPAT_CAPS ioctl

This commit adds documentation for the KVM_PPC_GET_COMPAT_CAPS ioctl, which
exposes host processor compatibility modes for nested KVM guests on PowerPC
systems. The documentation describes the structure, error codes, and the
extensible versioning mechanism using the size field.

> 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
[ ... ]
> +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.
> +``KVM_PPC_COMPAT_CAPS_SIZE_VER0`` (24) is a frozen constant marking the
> +size of the initial struct version.

[Severity: High]
Does the implementation of this extensible versioning in kvm_arch_vm_ioctl()
introduce a potential buffer overflow when userspace passes a smaller struct?

Looking at the code in 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);

If a userspace process compiled with an older, smaller version of the struct
(e.g., 24 bytes) calls the ioctl on a newer kernel where the struct has grown
to 32 bytes, copy_struct_to_user() will copy up to the user-provided usize
(24 bytes).

Because the kernel explicitly executes host_caps.size = sizeof(host_caps)
before copying, the first 8 bytes of the user's buffer are overwritten with
the kernel's larger size (32).

If userspace reuses this struct for another ioctl call, it would unknowingly
pass usize = 32. The kernel would trust this size and write 32 bytes to the
24-byte user buffer, causing an out-of-bounds write in userspace.

Should the kernel only write back its struct size when returning -E2BIG,
rather than on the successful path when the user's struct is smaller?

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