Re: [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] RISC-V: KVM: stale topei passed to
  __imsic_swfile_extirq_update()
- [High] RISC-V: KVM: imsic->swfile dereferenced before !imsic check in
  inject
- [Low] RISC-V: KVM: missing Fixes trailer on IMSIC TOPEI serialization
  fix
--

commit 322bd38f6016b31c1cdf5b875effdce4436f4b2f
Author: BillXiang <xiangwencheng@lanxincomputing.com>

> RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
>
> The TOPEI read-and-clear must be atomic with respect to
> interrupt injection (set_bit in EIP). Otherwise an
> interrupt injected between the topei read and the
> clear_bit could be silently dropped.
>
> Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>

[Severity: Low]
Since this fixes a race in the IMSIC software file TOPEI handling, should
it include:

  Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC")

> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> index 302938972aa0e..f4cc1691e0478 100644
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c
> @@ -526,26 +526,25 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
>  		kvm_riscv_aia_free_hgei(old_vsfile_cpu, old_vsfile_hgei);
>  }
>  
> -static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
> +static void __imsic_swfile_extirq_update(struct kvm_vcpu *vcpu, u32 topei)
>  {
>  	struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>  	struct imsic_mrif *mrif = imsic->swfile;
> -	unsigned long flags;
> -
> -	/*
> -	 * The critical section is necessary during external interrupt
> -	 * updates to avoid the risk of losing interrupts due to potential
> -	 * interruptions between reading topei and updating pending status.
> -	 */
> -
> -	raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>  
> -	if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) &&
> -	    imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis))
> +	if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) && topei)
>  		kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT);
>  	else
>  		kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_VS_EXT);
> +}
> +
> +static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
> +{
> +	struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
> +	struct imsic_mrif *mrif = imsic->swfile;
> +	unsigned long flags;
>  
> +	raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
> +	__imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
>  	raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
>  }
>  
> @@ -830,12 +829,15 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>  	struct imsic_mrif_eix *eix;
>  	int r, rc = KVM_INSN_CONTINUE_NEXT_SEPC;
>  	struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
> +	unsigned long flags;
>  
>  	/* If IMSIC vCPU state not initialized then forward to user space */
>  	if (!imsic)
>  		return KVM_INSN_EXIT_TO_USER_SPACE;
>  
>  	if (isel == KVM_RISCV_AIA_IMSIC_TOPEI) {
> +		raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
> +
>  		/* Read pending and enabled interrupt with highest priority */
>  		topei = imsic_mrif_topei(imsic->swfile, imsic->nr_eix,
>  					 imsic->nr_msis);
> @@ -852,16 +854,20 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>  					  eix->eip);
>  			}
>  		}
> +		if (wr_mask)
> +			__imsic_swfile_extirq_update(vcpu, topei);

[Severity: High]
Does this pass the pre-clear interrupt ID in topei to
__imsic_swfile_extirq_update() instead of re-reading imsic_mrif_topei()
after clear_bit()?

When a guest claims the last pending interrupt in imsic->swfile via a
TOPEI write, topei still holds the non-zero ID that was just cleared from
eix->eip.  __imsic_swfile_extirq_update() then calls
kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT) instead of unsetting
IRQ_VS_EXT, leaving IRQ_VS_EXT asserted and causing spurious guest
external interrupt traps.

[ ... ]

> @@ -959,6 +965,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>  	unsigned long flags;
>  	struct imsic_mrif_eix *eix;
>  	struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
> +	struct imsic_mrif *mrif = imsic->swfile;
>  
>  	/* We only emulate one IMSIC MMIO page for each Guest VCPU */
>  	if (!imsic || !iid || guest_index ||

[Severity: High]
Does imsic->swfile dereference a NULL imsic pointer before the !imsic
check below?  If a vCPU is created after aia_init(), its imsic_state is
NULL and injecting an MSI to that vCPU will crash the host kernel instead
of returning -ENODEV.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113101358.3007601-1-xiangwencheng@lanxincomputing.com?part=1