Re: [PATCH v2 18/20] KVM: s390: arm64: Implement SVE for arm guests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] SVE register accessors copy fewer bytes than the KVM ABI specifies, failing to zero-pad the remainder of the userspace buffer.
- [Medium] SVE ioctls and vCPU finalization bypass feature gating checks.
- [Low] Unrecognized or invalid SVE register IDs result in a -EFAULT return code instead of -ENOENT or -EINVAL.
--

commit 7e278493890194c05cc617a90bf08952cf400bea
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: arm64: Implement SVE for arm guests
    
Add support for Scalable Vector Extension (SVE) in ARM64 guests running
on s390 hardware, providing 128-bit vector operations backed by s390's
Vector Extension (VX).

> diff --git a/arch/s390/kvm/arm64/guest.c b/arch/s390/kvm/arm64/guest.c
> index 0474609759360..63fc4a64ab48f 100644
> --- a/arch/s390/kvm/arm64/guest.c
> +++ b/arch/s390/kvm/arm64/guest.c
[ ... ]
> +static inline int get_sve_ffr_reg(struct kvm_vcpu *vcpu, unsigned int regnum,
> +				  u16 __user *uptr)
> +{
> +	/* ffr is pregmax + 1 */
> +	if (regnum != KVM_ARM64_SVE_NUM_PREGS)
> +		return -EINVAL;
> +
> +	if (put_user(vcpu->arch.sae_block.sve_ffr, uptr))

[Severity: Medium]
The ARM64 KVM ABI defines FFR and PREG registers as 32 bytes
(KVM_REG_SIZE_U256), but this only writes 2 bytes via the u16 pointer. Will
this leave 30 bytes of the userspace buffer uninitialized, potentially
leaking memory contents into the migration stream when QEMU reads it?

> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static inline int get_sve_preg(struct kvm_vcpu *vcpu, unsigned int regnum,
> +			       u16 __user *uptr)
> +{
> +	if (regnum < 0 || regnum >= KVM_ARM64_SVE_NUM_PREGS)
> +		return -EINVAL;
> +
> +	if (put_user(vcpu->arch.sae_block.sve_pregs[regnum], uptr))

[Severity: Medium]
Similar to get_sve_ffr_reg(), this only copies 2 bytes for a register size
defined as 32 bytes. Should this zero-pad the rest of the buffer according to
the ABI?

> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static inline int get_sve_zreg(struct kvm_vcpu *vcpu, unsigned int regnum,
> +			       __vector128 __user *uptr)
> +{
> +	if (regnum < 0 || regnum >= KVM_ARM64_SVE_NUM_ZREGS)
> +		return -EINVAL;
> +
> +	/* vreg and svreg overlap and zreg is also just 128 bit so we reuse the vreg space */
> +	if (copy_to_user(uptr, &vcpu->arch.ctxt.vregs[regnum],
> +			 sizeof(vcpu->arch.ctxt.vregs[regnum])))

[Severity: Medium]
The ZREG size is defined as 256 bytes (KVM_REG_SIZE_U2048) by the ABI, but
this only copies 16 bytes. Does this leave the remaining 240 bytes of the
userspace buffer untouched, breaking ABI expectations?

> +		return -EFAULT;
> +	return 0;
> +}
[ ... ]
> +static int get_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	void __user *uptr = (void __user *)reg->addr;
> +	unsigned int regnum;
> +	int ret = -EFAULT;
> +
> +	if (reg->id == KVM_REG_ARM64_SVE_VLS)
> +		ret = get_sve_vls(vcpu, uptr);
> +	else if (sve_ffr_index(reg->id, &regnum) >= 0)
> +		ret = get_sve_ffr_reg(vcpu, regnum, uptr);
> +	else if (sve_preg_index(reg->id, &regnum) >= 0)
> +		ret = get_sve_preg(vcpu, regnum, uptr);
> +	else if (sve_zreg_index(reg->id, &regnum) >= 0)
> +		ret = get_sve_zreg(vcpu, regnum, uptr);

[Severity: Medium]
Does this function need to enforce the vcpu_has_sve() and
kvm_arm_vcpu_sve_finalized() constraints before delegating to the register
accessors, as required by the ABI?

[Severity: Low]
If an unrecognized or invalid SVE register ID is passed, the index validators
(e.g. sve_zreg_index()) return negative error codes like -ENOENT or -EINVAL,
but this function ignores them and falls through to returning the default
initialized ret = -EFAULT. Is it intentional to return -EFAULT instead of
propagating the specific error codes, which userspace like QEMU might rely
on to detect the end of slices?

> +
> +	return ret;
> +}
> +
> +static int set_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> +	const void __user *uptr = (const void __user *)reg->addr;
> +	int ret = -EFAULT;
> +	unsigned int regnum;
> +
> +	if (reg->id == KVM_REG_ARM64_SVE_VLS)
> +		ret = set_sve_vls(vcpu, uptr);
> +	else if (sve_ffr_index(reg->id, &regnum) >= 0)
> +		ret = set_sve_ffr_reg(vcpu, regnum, uptr);
> +	else if (sve_preg_index(reg->id, &regnum) >= 0)
> +		ret = set_sve_preg(vcpu, regnum, uptr);
> +	else if (sve_zreg_index(reg->id, &regnum) >= 0)
> +		ret = set_sve_zreg(vcpu, regnum, uptr);

[Severity: Medium]
Like get_sve_reg(), should this also check if the vCPU has the SVE feature
enabled and finalized before allowing state modifications to vector registers?

> +
> +	return ret;
> +}
> +
> diff --git a/arch/s390/kvm/arm64/reset.c b/arch/s390/kvm/arm64/reset.c
> index bc7bcee75779e..754c4d4a70b6b 100644
> --- a/arch/s390/kvm/arm64/reset.c
> +++ b/arch/s390/kvm/arm64/reset.c
[ ... ]
>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
>  {
> -	return 0;
> +	switch (feature) {
> +	case KVM_ARM_VCPU_SVE:
> +		if (!cpu_has_vx())
> +			return -EINVAL;
> +		if (vcpu_get_flag(vcpu, VCPU_SVE_FINALIZED))
> +			return -EPERM;

[Severity: Medium]
Should this function verify that the KVM_ARM_VCPU_SVE feature is actually
enabled for this vCPU (e.g. via vcpu_has_feature()) before allowing
finalization? Without this check, userspace could potentially finalize SVE
on non-SVE vCPUs.

> +		vcpu_set_flag(vcpu, VCPU_SVE_FINALIZED);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
>  }

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