Re: [PATCH] RISC-V: KVM: Fix timer state restore

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a strict UAPI register restoration ordering dependency, causing the timer to permanently stall if `state` is restored before `compare`.

Pre-existing issues:
- [Critical] Timer teardown uses a state flag for synchronization instead of unconditionally canceling the hrtimer, leading to a use-after-free on VCPU destruction.
- [High] Restoring the timer state to OFF fails with `-EINVAL` if the timer is already OFF, aborting live migration. This is exacerbated by SSTC incorrectly reporting its state.
--

commit 12ffaa755d54118c3c86ae260e5df5a3b6b59454
Author: Qiang Ma <maqianga@uniontech.com>

RISC-V: KVM: Fix timer state restore

This commit changes the timer state restoration logic in the one-reg interface
to re-arm the timer using the restored compare value (t->next_cycles) instead
of using the state value itself.

> diff --git a/arch/riscv/kvm/vcpu_timer.c b/arch/riscv/kvm/vcpu_timer.c
> --- a/arch/riscv/kvm/vcpu_timer.c
> +++ b/arch/riscv/kvm/vcpu_timer.c
> @@ -229,7 +229,7 @@ int kvm_riscv_vcpu_set_reg_timer(struct kvm_vcpu *vcpu,
>  		break;
>  	case KVM_REG_RISCV_TIMER_REG(state):
>  		if (reg_val == KVM_RISCV_TIMER_STATE_ON)
> -			ret = kvm_riscv_vcpu_timer_next_event(vcpu, reg_val);
> +			ret = kvm_riscv_vcpu_timer_next_event(vcpu, t->next_cycles);

[Severity: High]
Does this change introduce a strict ordering dependency for userspace register
restoration?

If a VMM restores the state register to ON before restoring the compare
register, it appears this will arm the timer using an uninitialized or stale
t->next_cycles value.

If the compare register is restored later via KVM_SET_ONE_REG:

	case KVM_REG_RISCV_TIMER_REG(compare):
		t->next_cycles = reg_val;
		break;

This only updates memory and doesn't seem to re-arm the running hrtimer. Will
this cause the virtual timer to stall indefinitely?

Before this patch, using 1 armed the timer for immediate expiration, and the
subsequent callback would asynchronously pick up the restored compare value.

>  		else
>  			ret = kvm_riscv_vcpu_timer_cancel(t);

[Severity: High]
This isn't a bug introduced by this patch, but does restoring the timer state
to OFF fail and abort live migration if the timer is already OFF?

If kvm_riscv_vcpu_timer_cancel() is called when restoring an OFF state:

static int kvm_riscv_vcpu_timer_cancel(struct kvm_vcpu_timer *t)
{
	if (!t->init_done || !t->next_set)
		return -EINVAL;
        ...
}

On the migration destination, a newly created VCPU has t->next_set == false, so
restoring an OFF state returns -EINVAL.

Additionally, when the SSTC extension is enabled, the timer is programmed
directly via CSRs and t->next_set is not set to true for a running timer. Will
this cause KVM_GET_ONE_REG to incorrectly report the state as OFF for active
VMs, which then fails during restoration on the destination?

[Severity: Critical]
This is a pre-existing issue, but does using the t->next_set flag for
synchronization during teardown lead to a use-after-free?

Looking at kvm_riscv_vcpu_timer_cancel(), it skips canceling the hrtimer if
the flag is false:

static int kvm_riscv_vcpu_timer_cancel(struct kvm_vcpu_timer *t)
{
	if (!t->init_done || !t->next_set)
		return -EINVAL;

	hrtimer_cancel(&t->hrt);
	t->next_set = false;
...

If the hrtimer callback clears t->next_set before it finishes executing, and
the VCPU is destroyed concurrently, the teardown path will see
t->next_set == false and skip hrtimer_cancel().

Will the still-executing callback then dereference the freed VCPU memory?

>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526075544.796396-1-maqianga@uniontech.com?part=1