Re: [PATCH v5 1/4] KVM: PPC: Introduce KVM_CAP_PPC_COMPAT_CAPS and wire up ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The ioctl overwrites the userspace structure's `size` field with a value larger than the allocated buffer on success.
- [Medium] The explicit rejection of larger userspace structs defeats the built-in forward compatibility mechanism of `copy_struct_from_user()`.
--

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

KVM: PPC: Introduce KVM_CAP_PPC_COMPAT_CAPS and wire up ioctl

This commit introduces a new capability and ioctl to expose CPU
compatibility modes supported by the host processor for nested guests.
It defines struct kvm_ppc_compat_caps and adds ioctl handling via
KVM_PPC_GET_COMPAT_CAPS.

> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 00302399fc37b..8d6632c64ca5f 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
[ ... ]
> @@ -2463,6 +2470,70 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
[ ... ]
> +		/*
> +		 * New userspace with a larger struct called an older kernel.
> +		 * Write back ksize in host_caps.size so userspace knows which
> +		 * older struct to retry with, then fail with -E2BIG.
> +		 */
> +		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;
> +		}
> +
> +		/*
> +		 * copy_struct_from_user() handles forward/backward compat:
> +		 *   usize == ksize: verbatim copy
> +		 *   usize <  ksize: zero-pad trailing (old userspace, new kernel)
> +		 */
> +		r = copy_struct_from_user(&host_caps, sizeof(host_caps),
> +					  argp, usize);

[Severity: Medium]
Does the explicit rejection of larger userspace structs defeat the built-in
forward compatibility mechanism of copy_struct_from_user()?

The copy_struct_from_user() helper natively handles larger userspace structs by
succeeding if the trailing bytes are zero, allowing a newer userspace to
transparently run on an older kernel without manual fallback logic. By
unconditionally returning -E2BIG here in kvm_arch_vm_ioctl() when usize is
greater than sizeof(host_caps), doesn't this force userspace to implement
explicit retry mechanisms instead of relying on the standard behavior?

[ ... ]
> +		r = kvm->arch.kvm_ops->get_compat_caps(&host_caps);
> +		if (r)
> +			goto out;
> +
> +		host_caps.size = sizeof(host_caps);
> +		r = copy_struct_to_user(argp, usize, &host_caps,
> +					sizeof(host_caps), NULL);

[Severity: High]
If an older userspace passes a smaller struct where usize is less than
sizeof(host_caps), does unconditionally setting host_caps.size to the larger
kernel size overwrite the userspace size field with a value larger than its
allocated buffer?

If userspace relies on this returned size field to process the structure,
could reading this larger size cause it to read past the end of its
allocated buffer?

> +		break;
> +	}

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