Re: [PATCH v6 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 3 potential issue(s) to consider:
- [High] The ioctl artificially inflates the userspace struct's `size` field on success, creating a memory corruption trap if userspace reuses the struct.
- [High] The ioctl unconditionally rejects larger structs with -E2BIG, bypassing standard UAPI zero-checking and encouraging silent input truncation.
- [Medium] The KVM_PPC_GET_COMPAT_CAPS ioctl availability is decoupled from its KVM_CAP_PPC_COMPAT_CAPS capability, breaking capability negotiation and bisectability.
--

--- Patch [1]: [PATCH v6 1/4] KVM: PPC: Introduce KVM_CAP_PPC_COMPAT_CAPS and wire up ioctl ---
commit bcf86bb5d3079ca2dbb4bf09693ba6e6a24b06c5
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.

> 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
> @@ -697,6 +697,13 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  			}
>  		}
>  		break;
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE)
> +	case KVM_CAP_PPC_COMPAT_CAPS:
> +		r = 0;
> +		if (kvmhv_on_pseries())
> +			r = 1;
> +		break;

[Severity: Medium]
Is this capability check fully aligned with the ioctl's availability?

This check returns 1 on a pseries host using PR KVM (since it misses the
hv_enabled check), but the ioctl might fail with -ENOTTY if the backend
is missing.

Later in the ioctl execution, it succeeds on a bare-metal host using HV KVM
(missing the kvmhv_on_pseries() check) even though this capability check
would return 0.

Could this decoupling between the capability and the ioctl availability break
capability negotiation for userspace?

> +#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
>  	default:
>  		r = 0;
>  		break;

[ ... ]

> @@ -2463,6 +2470,70 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  		r = kvm->arch.kvm_ops->svm_off(kvm);
>  		break;
>  	}
> +	case KVM_PPC_GET_COMPAT_CAPS: {
> +		struct kvm_ppc_compat_caps host_caps = {};
> +		u64 usize;
> +
> +		/*
> +		 * Read the size field first to drive copy_struct_from_user.
> +		 * size must be the first field of the struct.
> +		 */
> +		r = -EFAULT;
> +		if (get_user(usize, (__u64 __user *)argp))
> +			goto out;
> +
> +		/*
> +		 * Enforce a minimum: reject buffers smaller than the initial
> +		 * struct version (VER0). This allows old userspace compiled
> +		 * against the original struct to still work on a newer kernel
> +		 * that has grown the struct with appended fields.
> +		 */
> +		r = -EINVAL;
> +		if (usize < KVM_PPC_COMPAT_CAPS_SIZE_VER0)
> +			goto out;
> +
> +		/*
> +		 * 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;
> +		}

[Severity: High]
Does explicitly rejecting larger structs here bypass the standard UAPI
zero-checking?

By catching usize > sizeof(host_caps) early and returning -E2BIG, the code
bypasses copy_struct_from_user()'s built-in validation which would safely
reject non-zero extended fields.

If userspace receives the smaller kernel size and retries by truncating its
buffer size, the kernel will only read the truncated portion, silently
ignoring any non-zero extended fields that userspace originally provided.

Should this rely on copy_struct_from_user() to safely check zeroed trailing
bytes instead?

> +
> +		/*
> +		 * 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);
> +		if (r)
> +			goto out;
> +
> +		/* Reserved fields must be zero */
> +		r = -EINVAL;
> +		if (host_caps.flags)
> +			goto out;
> +
> +		r = -ENOTTY;
> +		if (!kvm->arch.kvm_ops->get_compat_caps)
> +			goto out;
> +
> +		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]
Can this artificial size inflation cause an out-of-bounds read/write if
userspace reuses the struct?

If an older userspace passes a smaller struct to a newer kernel (where
usize < sizeof(host_caps)), the kernel sets host_caps.size to its own larger
size. It then copies the smaller usize bytes back, overwriting the userspace
size field with the kernel's larger size.

If userspace reuses this struct for a subsequent ioctl call, it will now pass
the larger size. The kernel would then read past the end of the originally
allocated userspace buffer, and if successful, write back past the end
as well.

> +		break;
> +	}
>  	default: {
>  		struct kvm *kvm = filp->private_data;

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