Re: [PATCH 2/2] KVM: riscv: Eliminate redundancy in set_reg function via pointers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: riscv: wrong FP register size check in kvm_riscv_vcpu_set_reg_fp()
--

commit 22c4a5d5b5662dbf020a9f9d6d213a5477e96b0d
Author: Paul White <paul.white.kernel@gmail.com>

KVM: riscv: Eliminate redundancy in set_reg function via pointers

This commit refactors kvm_riscv_vcpu_set_reg_fp() to consolidate the register
type and bounds checks for single-precision and double-precision floating
point registers using local pointer and offset variables.

> diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> index 16999a5c45dec..fe49576035192 100644
> --- a/arch/riscv/kvm/vcpu_fp.c
> +++ b/arch/riscv/kvm/vcpu_fp.c
> @@ -144,41 +144,53 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
>  	struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
>  	unsigned long __user *uaddr =
>  			(unsigned long __user *)(unsigned long)reg->addr;
> -	unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> +	unsigned long token_reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
>  					    KVM_REG_SIZE_MASK |
>  					    rtype);
>  	void *reg_val;
>  
> +	u32 *fcsr_addr;
> +	void *f_array_addr;
> +	size_t byte_offset = 0;
> +	unsigned long offical_fcsr_num = 0;
> +	unsigned long offical_first_reg = 0;
> +	unsigned long offical_last_reg = 0;
> +
>  	if ((rtype == KVM_REG_RISCV_FP_F) &&
>  	    riscv_isa_extension_available(vcpu->arch.isa, F)) {
> +		fcsr_addr = &cntx->fp.f.fcsr;
> +		f_array_addr = &cntx->fp.f.f;
> +		byte_offset = 4;
> +		offical_fcsr_num = KVM_REG_RISCV_FP_F_REG(fcsr);
> +		offical_first_reg = KVM_REG_RISCV_FP_F_REG(f[0]);
> +		offical_last_reg = KVM_REG_RISCV_FP_F_REG(f[31]);
> +	} else if  ((rtype == KVM_REG_RISCV_FP_D) &&
> +			   riscv_isa_extension_available(vcpu->arch.isa, D)) {
> +		fcsr_addr = &cntx->fp.d.fcsr;
> +		f_array_addr = &cntx->fp.d.f;
> +		byte_offset = 8;
> +		offical_fcsr_num = KVM_REG_RISCV_FP_D_REG(fcsr);
> +		offical_first_reg = KVM_REG_RISCV_FP_D_REG(f[0]);
> +		offical_last_reg = KVM_REG_RISCV_FP_D_REG(f[31]);
> +	} else {
> +		return -ENOENT;
> +	}
> +
> +	if (token_reg_num == offical_fcsr_num) {
>  		if (KVM_REG_SIZE(reg->id) != sizeof(u32))
>  			return -EINVAL;
> -		if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> -			reg_val = &cntx->fp.f.fcsr;
> -		else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
> -			 reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
> -			reg_num = array_index_nospec(reg_num,
> -						     ARRAY_SIZE(cntx->fp.f.f));
> -			reg_val = &cntx->fp.f.f[reg_num];
> -		} else
> -			return -ENOENT;
> -	} else if ((rtype == KVM_REG_RISCV_FP_D) &&
> -		   riscv_isa_extension_available(vcpu->arch.isa, D)) {
> -		if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> -			if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> -				return -EINVAL;
> -			reg_val = &cntx->fp.d.fcsr;
> -		} else if ((KVM_REG_RISCV_FP_D_REG(f[0]) <= reg_num) &&
> -			   reg_num <= KVM_REG_RISCV_FP_D_REG(f[31])) {
> -			if (KVM_REG_SIZE(reg->id) != sizeof(u64))
> -				return -EINVAL;
> -			reg_num = array_index_nospec(reg_num,
> -						     ARRAY_SIZE(cntx->fp.d.f));
> -			reg_val = &cntx->fp.d.f[reg_num];
> -		} else
> -			return -ENOENT;
> -	} else
> +		reg_val = fcsr_addr;
> +	} else if ((offical_first_reg <= token_reg_num) && (token_reg_num <= offical_last_reg)) {
> +		if ((KVM_REG_SIZE(reg->id) != sizeof(u32)) &&
> +		    (KVM_REG_SIZE(reg->id) != sizeof(u64))) {
> +			return -EINVAL;
> +		}
> +		token_reg_num = array_index_nospec(token_reg_num,
> +						   32);
> +		reg_val = (u8 *)f_array_addr + (token_reg_num * byte_offset);
> +	} else {
>  		return -ENOENT;
> +	}
>  
>  	if (copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id)))
>  		return -EFAULT;

[Severity: High]
Can this size check allow an out-of-bounds write to cntx->fp.f.f[] when rtype
is KVM_REG_RISCV_FP_F and userspace passes KVM_REG_SIZE_U64 in reg->id via
KVM_SET_ONE_REG?

Previously, kvm_riscv_vcpu_set_reg_fp() required KVM_REG_SIZE(reg->id) ==
sizeof(u32) for KVM_REG_RISCV_FP_F registers and KVM_REG_SIZE(reg->id) ==
sizeof(u64) for KVM_REG_RISCV_FP_D data registers f[0]..f[31].

With the combined check:

	if ((KVM_REG_SIZE(reg->id) != sizeof(u32)) &&
	    (KVM_REG_SIZE(reg->id) != sizeof(u64))) {
		return -EINVAL;
	}

an 8-byte size is accepted when rtype is KVM_REG_RISCV_FP_F (where
f_array_addr points to cntx->fp.f.f and byte_offset is 4). The subsequent
copy_from_user(reg_val, uaddr, KVM_REG_SIZE(reg->id)) then writes 8 bytes
into a 4-byte element of cntx->fp.f.f[32], overwriting the adjacent register
cntx->fp.f.f[token_reg_num + 1] or overflowing cntx->fp.f.f[] into
cntx->fp.f.fcsr when token_reg_num is 31.

Conversely, when rtype is KVM_REG_RISCV_FP_D (where byte_offset is 8), a
4-byte size is now accepted for f[0]..f[31] instead of returning -EINVAL,
resulting in a partial 4-byte write to cntx->fp.d.f[token_reg_num].

Note that the same check was also introduced in kvm_riscv_vcpu_get_reg_fp()
earlier in this series, where copy_to_user() can similarly read 8 bytes from
a 4-byte element of cntx->fp.f.f[32].

Should both functions check KVM_REG_SIZE(reg->id) != byte_offset for the data
registers instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792982932.git.paul.white.kernel@gmail.com?part=2