Re: [PATCH] RISC-V: KVM: Use raw_spinlock for VMID update critical section

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

Pre-existing issues:
- [Critical] Race condition during VMID rollover allows a vCPU to transiently execute in guest mode with a stale VMID, compromising cross-VM isolation.
- [High] Integer overflow in the VMID generation counter (vmid_version) on 32-bit systems causes a cross-VM isolation break.
--

--- Patch [1]: [PATCH] RISC-V: KVM: Use raw_spinlock for VMID update critical section ---
commit 7d899ea43321661351b3cdd958ba47c38d85eeed
Author: Yuhang.chen <[email protected]>

RISC-V: KVM: Use raw_spinlock for VMID update critical section

This commit converts the vmid_lock to a raw_spinlock_t to prevent
preemption during the VMID update section under PREEMPT_RT. The update
path performs an IPI broadcast which requires preemption to be disabled.

> diff --git a/arch/riscv/kvm/vmid.c b/arch/riscv/kvm/vmid.c
> index 3b426c800480c..33efde4bb9d75 100644
> --- a/arch/riscv/kvm/vmid.c
> +++ b/arch/riscv/kvm/vmid.c
> @@ -20,7 +20,7 @@
>  static unsigned long vmid_version = 1;
>  static unsigned long vmid_next;
>  static unsigned long vmid_bits __ro_after_init;
> -static DEFINE_SPINLOCK(vmid_lock);
> +static DEFINE_RAW_SPINLOCK(vmid_lock);
>  
>  void __init kvm_riscv_gstage_vmid_detect(void)
>  {

[Severity: High]
This is a pre-existing issue, but does this unsigned long counter present an
overflow risk on 32-bit systems?

If a host user rapidly creates and destroys VMs, the 32-bit vmid_version could
wrap back to 0. Since a new VM's kvm_vmid struct is zero-initialized,
kvm_riscv_gstage_vmid_ver_changed() would evaluate (0 != 0) as false:

kvm_riscv_gstage_vmid_ver_changed() {
    ...
    return unlikely(READ_ONCE(vmid->vmid_version) !=
                    READ_ONCE(vmid_version));
}

Could this cause multiple new VMs to share VMID 0, breaking cross-VM isolation
by allowing their memory accesses to hit each other's TLB entries?

[ ... ]

> @@ -118,7 +118,7 @@ void kvm_riscv_gstage_vmid_update(struct kvm_vcpu *vcpu)
>  
>  	WRITE_ONCE(vmid->vmid_version, READ_ONCE(vmid_version));
>  
> -	spin_unlock(&vmid_lock);
> +	raw_spin_unlock(&vmid_lock);
>  
>  	/* Request G-stage page table update for all VCPUs */
>  	kvm_for_each_vcpu(i, v, vcpu->kvm)
>  		kvm_make_request(KVM_REQ_UPDATE_HGATP, v);

[Severity: Critical]
This isn't a bug introduced by this patch, but does dropping the lock before
making the requests open a race window?

If the lock is released after updating vmid_version but before setting
KVM_REQ_UPDATE_HGATP, could a concurrent vCPU from the same VM observe the
new vmid_version without the lock?

If it sees no pending requests, it might proceed to enter the guest with a
stale VMID. If a global VMID rollover just occurred, could this stale VMID
have been re-assigned to a different VM, causing an isolation break?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1