Re: [PATCH v4 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 4 potential issue(s) to consider:
- [High] Backward compatibility failure due to strict size check and unconditional copy size.
- [High] Incorrect `_IOR` ioctl direction flag breaks UAPI extensibility and tooling.
- [High] Unvalidated reserved `flags` field prevents future use as an input.
- [Medium] Capability `KVM_CAP_PPC_COMPAT_CAPS` is advertised even when the backend lacks support.
--

commit c1e1544ff6eb8d147eda5713932bdc47b539c526
Author: Amit Machhiwal <amachhiw@linux.ibm.com>
Subject: KVM: PPC: Introduce KVM_CAP_PPC_COMPAT_CAPS and wire up ioctl

This commit introduces a new KVM capability and ioctl to expose host CPU
compatibility modes to nested guests. It adds the KVM_CAP_PPC_COMPAT_CAPS
capability and the KVM_PPC_GET_COMPAT_CAPS ioctl, which returns a bitmap
of supported compatibility modes via the kvm_ppc_compat_caps structure.

> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 00302399fc37b..2f72431b6ef53 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;
> +#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */

[Severity: Medium]
Will this falsely advertise support if the backend lacks the get_compat_caps
callback?

It appears this unconditionally returns 1 when kvmhv_on_pseries() is true,
even if running on a backend like KVM-PR that might not populate
kvm->arch.kvm_ops->get_compat_caps.

Should this verify the backend capabilities before returning 1?

[ ... ]
> @@ -2463,6 +2470,34 @@ 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 user_size;
> +
> +		r = -EFAULT;
> +		/* First, get the size field from userspace to validate */
> +		if (copy_from_user(&user_size, &((struct kvm_ppc_compat_caps
> +		     __user *)argp)->size, sizeof(user_size))) {
> +			goto out;
> +		}
> +
> +		/* Validate size - must be at least the current structure size */
> +		r = -EINVAL;
> +		if (user_size < sizeof(host_caps))
> +			goto out;

[Severity: High]
Does this strict size check prevent backward compatibility?

If the structure is extended in a future kernel, sizeof(host_caps) will
increase. Older userspace binaries passing their original, smaller user_size
will be rejected with -EINVAL.

Could this be resolved by accepting smaller sizes and limiting the
copy_to_user() length to min(user_size, sizeof(host_caps))?

> +
> +		r = -ENOTTY;
> +		memset(&host_caps, 0, sizeof(host_caps));

[Severity: High]
Does zeroing the structure locally without validating the user-provided flags
prevent those flags from being used in the future?

Since the userspace flags aren't read and verified to be zero, applications
might pass uninitialized garbage memory. If a future kernel assigns meaning to
these flags, wouldn't it interpret that legacy garbage data as valid input?

> +		if (!kvm->arch.kvm_ops->get_compat_caps)
> +			goto out;
> +
> +		r = kvm->arch.kvm_ops->get_compat_caps(&host_caps);
> +		/* Set the actual size of the structure we're returning */
> +		host_caps.size = sizeof(host_caps);
> +		if (!r && copy_to_user(argp, &host_caps, sizeof(host_caps)))
> +			r = -EFAULT;
> +		break;
> +	}
[ ... ]
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6c8afa2047bf3..1788a00686626 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
[ ... ]
> @@ -1349,6 +1350,9 @@ struct kvm_s390_keyop {
>  #define KVM_GET_DEVICE_ATTR	  _IOW(KVMIO,  0xe2, struct kvm_device_attr)
>  #define KVM_HAS_DEVICE_ATTR	  _IOW(KVMIO,  0xe3, struct kvm_device_attr)
>  
> +/* Available with KVM_CAP_PPC_COMPAT_CAPS */
> +#define KVM_PPC_GET_COMPAT_CAPS	_IOR(KVMIO,  0xe4, struct kvm_ppc_compat_caps)

[Severity: High]
Does using _IOR here cause issues for tracing tools and user-mode translation
layers?

Since the implementation reads the size field from userspace via
copy_from_user(), it seems this should be defined with _IOWR.

Also, since _IOR embeds the structure size into the ioctl number, won't
expanding the structure in the future change the ioctl number and break older
binaries?

>  /*
>   * ioctls for vcpu fds
>   */

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