Re: [PATCH 1/2] KVM: riscv: Eliminate redundancy in get_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: mismatched register size check in kvm_riscv_vcpu_get_reg_fp()
--

commit 0e99059ef6014093a2535ab4ecda318fe5ee3489
Author: Paul White <paul.white.kernel@gmail.com>

KVM: riscv: Eliminate redundancy in get_reg function via pointers

This commit refactors kvm_riscv_vcpu_get_reg_fp() to consolidate the
register lookup and size checks for single-precision and double-precision
floating-point registers using mode-dependent local variables and pointer
offsets.

> diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> index bb11e6757d349..16999a5c45dec 100644
> --- a/arch/riscv/kvm/vcpu_fp.c
> +++ b/arch/riscv/kvm/vcpu_fp.c
> @@ -83,41 +83,53 @@ int kvm_riscv_vcpu_get_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_to_user(uaddr, reg_val, KVM_REG_SIZE(reg->id)))
>  		return -EFAULT;

[Severity: High]
Can this read past the bounds of an element in cntx->fp.f.f[] when userspace
requests a single-precision register with a 64-bit size?

In kvm_riscv_vcpu_get_reg_fp(), the merged size check for floating-point data
registers allows both sizeof(u32) and sizeof(u64) regardless of whether rtype
is KVM_REG_RISCV_FP_F (where byte_offset is 4 and f_array_addr points to the
32-bit array cntx->fp.f.f) or KVM_REG_RISCV_FP_D (where byte_offset is 8 and
f_array_addr points to the 64-bit array cntx->fp.d.f).

If userspace invokes the vcpu ioctl path:

  kvm_arch_vcpu_ioctl() ->
    kvm_riscv_vcpu_get_reg() ->
      kvm_riscv_vcpu_get_reg_fp()

with rtype == KVM_REG_RISCV_FP_F and KVM_REG_SIZE(reg->id) == sizeof(u64),
the size check now passes instead of returning -EINVAL. Then copy_to_user()
copies 8 bytes from a 4-byte slot at (u8 *)f_array_addr + (token_reg_num * 4),
reading into the adjacent element of cntx->fp.f.f[] (or past cntx->fp.f.f[31]
into cntx->fp.f.fcsr). Conversely, requesting a KVM_REG_RISCV_FP_D data
register with sizeof(u32) also passes and copies only 4 bytes instead of
returning -EINVAL.

Should the size check here compare against byte_offset instead:

	if (KVM_REG_SIZE(reg->id) != byte_offset)
		return -EINVAL;

so that the register size must match the width of the selected floating-point
mode?

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