Re: [PATCH v3 12/17] riscv: kvm: Use generated instruction headers for mmio emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Signed left shift leading to Undefined Behavior in `kvm_riscv_vcpu_mmio_return`.
- [High] Incorrect register extraction for RISC-V compressed load and store instructions in KVM MMIO emulation leads to guest state corruption.

Pre-existing issues:
- [High] The `mmio_test_destroy` function calls `kvm_io_bus_unregister_dev` without holding the required `kvm->slots_lock`.
--

--- Patch [12]: [PATCH v3 12/17] riscv: kvm: Use generated instruction headers for mmio emulation ---
commit 8d80090faef41d1000ad8ee985dcdaa3df7c12d3
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: kvm: Use generated instruction headers for mmio emulation

Migrate the mmio emulation code to use the generated instruction headers
instead of the hand-written instruction composition functions.

> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> index a1898ab8bbe85..78adc28308193 100644
> --- a/arch/riscv/kvm/vcpu_insn.c
> +++ b/arch/riscv/kvm/vcpu_insn.c

[ ... ]

> -	} else if ((insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD) {
> +	} else if (riscv_insn_is_c_ld(insn)) {
>  		len = 8;
>  		shift = 8 * (sizeof(ulong) - len);
> -		insn = RVC_RS2S(insn) << SH_RD;
> -	} else if ((insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
> -		   ((insn >> SH_RD) & 0x1f)) {
> +		rd = riscv_insn_c_ld_extract_xd(insn);

[Severity: High]
Does kvm_riscv_vcpu_mmio_load miss the +8 register offset for compressed
instructions?

The old code used RVC_RS2S which mapped the 3-bit compressed register field
to architectural registers x8-x15. Without adding this offset, won't this
corrupt guest state by targeting registers x0-x7 instead?

[ ... ]

> -	} else if ((insn & INSN_MASK_C_SD) == INSN_MATCH_C_SD) {
> +	} else if (riscv_insn_is_c_sd(insn)) {
>  		len = 8;
> -		data64 = GET_RS2S(insn, &vcpu->arch.guest_context);
> -	} else if ((insn & INSN_MASK_C_SDSP) == INSN_MATCH_C_SDSP &&
> -		   ((insn >> SH_RD) & 0x1f)) {
> +		rs2 = riscv_insn_c_sd_extract_xs2(insn);

[Severity: High]
Similarly here in kvm_riscv_vcpu_mmio_store, does riscv_insn_c_sd_extract_xs2
also need the +8 offset added to map to the correct source register?

[ ... ]

> +		*((ulong *)(&vcpu->arch.guest_context) + vcpu->arch.mmio_decode.rd) =
> +			(long)data << shift >> shift;

[Severity: High]
Can this introduce undefined behavior in kvm_riscv_vcpu_mmio_return from a
signed left shift?

If a guest reads a value with the high bit set (like 0xFF for a 1-byte read),
casting data to long before the shift evaluates to 255 << 56. This
overflows the maximum positive value for a 64-bit signed integer.

The previous code safely did:
    (long)((ulong)data << shift) >> shift
    
Could we keep the unsigned cast before the left shift?

>  	}
>  
>  done:

> diff --git a/virt/kvm/mmio_test.c b/virt/kvm/mmio_test.c
> index bd5f21a43ba87..b3fec6988333f 100644
> --- a/virt/kvm/mmio_test.c
> +++ b/virt/kvm/mmio_test.c

[ ... ]

>  static void mmio_test_destroy(struct kvm_device *dev)
>  {
> -	struct mmio_test *mmio_test = kvm_to_mmio_test_dev(dev);
> -
> -	kvm_io_bus_unregister_dev(dev->kvm, KVM_MMIO_BUS, &mmio_test->dev);
> +	kvm_io_bus_unregister_dev(dev->kvm, KVM_MMIO_BUS, &((struct mmio_test *)dev->private)->dev);

[Severity: High]
This is a pre-existing issue, but does mmio_test_destroy need to hold
dev->kvm->slots_lock around this call?

kvm_io_bus_unregister_dev has a lockdep_assert_held for kvm->slots_lock.
Since mmio_test_destroy is called during kvm_destroy_devices without
holding this lock, won't this trigger a kernel lockdep splat when
userspace closes the VM?

>  	kfree(dev->private);
>  	kfree(dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-riscv_insn_table-v3-0-3f8e5f75ba97@gmail.com?part=12