Re: [PATCH] RISC-V: KVM: Fix NULL pointer dereference in AIA IMSIC functions

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Incomplete fix for VCPU creation after AIA initialization leaves multiple NULL pointer dereferences
- [High] Acquiring rwlock_t in preempt notifier context causes 'sleeping while atomic' panic on PREEMPT_RT
--

commit 1dfd448f59b0d23fa69ed899c4ce00ac3e71eeaa
Author: Jiakai Xu <xujiakai2025@iscas.ac.cn>

RISC-V: KVM: Fix NULL pointer dereference in AIA IMSIC functions

This commit fixes a NULL pointer dereference in the AIA IMSIC functions
by adding NULL pointer guards to kvm_riscv_vcpu_aia_imsic_put(), release(),
and has_interrupt(). The issue occurs when a VCPU is created after the AIA
device is initialized, which leaves its imsic_state NULL.

> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> index fda0346f0ea1f..e29cf9feb1ba3 100644
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c
[ ... ]
> @@ -710,6 +713,9 @@ void kvm_riscv_vcpu_aia_imsic_put(struct kvm_vcpu *vcpu)
>  	struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>  	unsigned long flags;
>  
> +	if (!imsic)
> +		return;
> +
>  	if (!kvm_vcpu_is_blocking(vcpu))
>  		return;
>  

[Severity: High]
This is a pre-existing issue, but is it safe to acquire an rwlock_t inside
kvm_riscv_vcpu_aia_imsic_put()?

This function eventually calls read_lock_irqsave(&imsic->vsfile_lock, flags)
and is called from the preempt notifier context:

context_switch()
  prepare_task_switch()
    fire_sched_out_preempt_notifiers()
      kvm_sched_out()
        kvm_arch_vcpu_put()
          kvm_riscv_vcpu_aia_put()
            kvm_riscv_vcpu_aia_imsic_put()

The preempt notifiers execute with the runqueue raw_spinlock_t held and
hardware interrupts disabled.

On PREEMPT_RT kernels, rwlock_t is mapped to a sleepable lock. Attempting
to acquire a sleepable lock while holding a raw spinlock causes a
scheduling while atomic kernel panic on any VCPU thread preemption.

[ ... ]
> @@ -726,6 +732,9 @@ void kvm_riscv_vcpu_aia_imsic_release(struct kvm_vcpu *vcpu)
>  	int old_vsfile_hgei, old_vsfile_cpu;
>  	struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>  
> +	if (!imsic)
> +		return;
> +
>  	/* Read and clear IMSIC VS-file details */
>  	write_lock_irqsave(&imsic->vsfile_lock, flags);
>  	old_vsfile_hgei = imsic->vsfile_hgei;

[Severity: Critical]
This is a pre-existing issue, but does this incomplete fix leave other paths
vulnerable to the exact same NULL pointer dereference?

The commit message notes that creating a VCPU after AIA initialization
leaves imsic_state NULL. While this patch protects the put, release, and
has_interrupt paths, other functions in the same file still unconditionally
dereference imsic_state under the same conditions.

For example, kvm_riscv_vcpu_aia_imsic_update() (called during the KVM_RUN
ioctl), kvm_riscv_vcpu_aia_imsic_rmw() (called during guest CSR access
traps), and kvm_riscv_aia_imsic_rw_attr() / kvm_riscv_aia_imsic_has_attr()
(called via KVM device ioctls).

Could a userspace program trigger a host kernel panic by initializing the
AIA device, creating a late VCPU, and then invoking KVM_RUN or
KVM_HAS_DEVICE_ATTR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526031517.1166025-1-xujiakai2025@iscas.ac.cn?part=1