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

On 2026/08/04 06:19 PM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

Looking closely, it looks like a good catch. PPC kernels can be built
both KVM-PR and as well KVM-HV config enabled. Though at one point in
time only one of kvm-pr and kvm-hv can be loaded, a PR KVM guest on a
pseries system can see cap=1 but then get -ENOTTY from the ioctl
(thought the documentation explicitly covers this case: "ENOTTY if the
backend does not implement the get_compat_caps operation (e.g., on
non-HV KVM implementations). But anyway, I'll fix in v7:

	case KVM_CAP_PPC_COMPAT_CAPS:
		r = 0;
		if (hv_enabled && kvmhv_on_pseries())
			r = 1;
		break;

> 
> 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.

On bare-metal, kvmhv_on_pseries() is false (CPU_FTR_HVMODE is set), so
the cap correctly returns 0 there regardless of hv_enabled. The ioctl is
only meaningful on pseries (PAPR guests) where KVM acts as L1 under a
hypervisor, which is why both conditions are required

> 
> 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?

The pre-check `usize > sizeof(host_caps)` is required, not a bypass. The
-E2BIG path writes back ksize so userspace can discover the kernel's
size and retry explicitly. Userspace that retries with the smaller size
is making a deliberate choice to use the older interface; the kernel
cannot have seen the truncated extra fields, so there is nothing
silently ignored. copy_struct_from_user() itself would also return
-E2BIG for non-zero trailing bytes — but it cannot be called safely with
an arbitrary usize against a fixed-size kernel struct.

> 
> 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.

Not a bug. `copy_struct_to_user(argp, usize, &host_caps,
sizeof(host_caps), NULL)` copies min(usize, ksize) bytes back to
userspace. Today usize == ksize == 24, so exactly 24 bytes are written.
In a future kernel where ksize grows, the function correctly copies only
usize bytes (old userspace case), meaning userspace sees its own size
field reflected back — not the kernel's larger size. The kernel sets
host_caps.size = sizeof(host_caps) in the kernel-side copy so that the
output `size` field documents the kernel's struct version when it is
within the copied range, which is the designed semantics for versioned
structs. No out-of-bounds access occurs.

> 
> > +		break;
> > +	}
> >  	default: {
> >  		struct kvm *kvm = filp->private_data;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1