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

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

[Qiang Ma]

The KVM_REG_RISCV_TIMER_REG(state) one-reg write passes the value
written by userspace to kvm_riscv_vcpu_timer_next_event() when
re-enabling the timer.

That value is the timer state, KVM_RISCV_TIMER_STATE_ON, not the
timer compare value. During migration or state restore, userspace
restores the compare register separately, which stores the target
cycle in t->next_cycles. Re-arming the timer with the state value
schedules the next event at cycle 1 instead of the restored compare
value, causing the virtual timer to fire too early.

Use the restored compare value from t->next_cycles when turning the
timer back on.

Fixes: 3a9f66cb25e1 ("RISC-V: KVM: Add timer functionality")

Signed-off-by: Qiang Ma <maqianga@uniontech.com>
---
 arch/riscv/kvm/vcpu_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kvm/vcpu_timer.c b/arch/riscv/kvm/vcpu_timer.c
index 9817ff802821..ae53133c7ab0 100644
--- a/arch/riscv/kvm/vcpu_timer.c
+++ b/arch/riscv/kvm/vcpu_timer.c
@@ -231,7 +231,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);
 		else
 			ret = kvm_riscv_vcpu_timer_cancel(t);
 		break;
-- 
2.20.1

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

[sashiko-bot]

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

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

[Qiang Ma]

在 2026/5/26 16:23, 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?
In qemu, in the kvm_riscv_put_regs_time(), the values were originally 
set in the order of time, compare, and state.
>
> 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?
Currently, such a situation does not occur in qemu.
>
> 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?
In QEMU, the VM is running normally and does not call the 
kvm_riscv_get_regs_timer().
>
> [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;

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

[Anup Patel]

On Tue, May 26, 2026 at 1:26 PM Qiang Ma <maqianga@uniontech.com> wrote:
>
> The KVM_REG_RISCV_TIMER_REG(state) one-reg write passes the value
> written by userspace to kvm_riscv_vcpu_timer_next_event() when
> re-enabling the timer.
>
> That value is the timer state, KVM_RISCV_TIMER_STATE_ON, not the
> timer compare value. During migration or state restore, userspace
> restores the compare register separately, which stores the target
> cycle in t->next_cycles. Re-arming the timer with the state value
> schedules the next event at cycle 1 instead of the restored compare
> value, causing the virtual timer to fire too early.
>
> Use the restored compare value from t->next_cycles when turning the
> timer back on.
>
> Fixes: 3a9f66cb25e1 ("RISC-V: KVM: Add timer functionality")
>
> Signed-off-by: Qiang Ma <maqianga@uniontech.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Queued this patch for Linux-7.2

Thanks,
Anup

> ---
>  arch/riscv/kvm/vcpu_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kvm/vcpu_timer.c b/arch/riscv/kvm/vcpu_timer.c
> index 9817ff802821..ae53133c7ab0 100644
> --- a/arch/riscv/kvm/vcpu_timer.c
> +++ b/arch/riscv/kvm/vcpu_timer.c
> @@ -231,7 +231,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);
>                 else
>                         ret = kvm_riscv_vcpu_timer_cancel(t);
>                 break;
> --
> 2.20.1
>

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

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Anup Patel <anup@brainfault.org>:

On Tue, 26 May 2026 15:55:44 +0800 you wrote:
> The KVM_REG_RISCV_TIMER_REG(state) one-reg write passes the value
> written by userspace to kvm_riscv_vcpu_timer_next_event() when
> re-enabling the timer.
> 
> That value is the timer state, KVM_RISCV_TIMER_STATE_ON, not the
> timer compare value. During migration or state restore, userspace
> restores the compare register separately, which stores the target
> cycle in t->next_cycles. Re-arming the timer with the state value
> schedules the next event at cycle 1 instead of the restored compare
> value, causing the virtual timer to fire too early.
> 
> [...]

Here is the summary with links:
  - RISC-V: KVM: Fix timer state restore
    https://git.kernel.org/riscv/c/6d4c17ed5620

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html