Re: [PATCH v1 1/3] RISC-V: KVM: Preserve firmware counter value across stop/start

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

Pre-existing issues:
- [Critical] The guest can trigger a host kernel panic and memory leaks by intentionally failing the shared memory setup.
- [High] The `sbiret` error variable is not reset per iteration in the `for_each_set_bit` loop of `kvm_riscv_vcpu_pmu_ctr_stop`, causing subsequent successfully stopped counters to silently drop their snapshot updates.
--

--- Patch [1]: [PATCH v1 1/3] RISC-V: KVM: Preserve firmware counter value across stop/start ---
commit 1ea35d28d13ff19e21fbca9d91839aa79150c9a5
Author: SeungJu Cheon <[email protected]>

RISC-V: KVM: Preserve firmware counter value across stop/start

Firmware events accumulate in kvpmu->fw_event[].value while running,
but counter stop only clears fw_event[].started without saving the
value back to pmc->counter_val. A subsequent counter start without
SBI_PMU_START_FLAG_SET_INIT_VALUE reloads the stale counter_val into
fw_event[].value, losing all events counted so far.

> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 2025b664961c8..56dd5b1b2846c 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -671,10 +671,12 @@ int kvm_riscv_vcpu_pmu_ctr_stop(struct kvm_vcpu *vcpu, unsigned long ctr_base,
>  				goto out;
>  			}
>  
> -			if (!kvpmu->fw_event[fevent_code].started)
> +			if (!kvpmu->fw_event[fevent_code].started) {
>  				sbiret = SBI_ERR_ALREADY_STOPPED;
> -
> -			kvpmu->fw_event[fevent_code].started = false;
> +			} else {
> +				kvpmu->fw_event[fevent_code].started = false;
> +				pmc->counter_val = kvpmu->fw_event[fevent_code].value;
> +			}
>  		} else if (pmc->perf_event) {
>  			if (pmc->started) {
>  				/* Stop counting the counter */
> @@ -692,9 +694,7 @@ int kvm_riscv_vcpu_pmu_ctr_stop(struct kvm_vcpu *vcpu, unsigned long ctr_base,
>  		}
>  
>  		if (snap_flag_set && !sbiret) {

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for sbiret to
remain set for subsequent iterations of the loop?

Looking at kvm_riscv_vcpu_pmu_ctr_stop() in arch/riscv/kvm/vcpu_pmu.c, if the
guest provides a counter mask with multiple counters and an early counter is
already stopped, sbiret is set to SBI_ERR_ALREADY_STOPPED.

Because sbiret isn't reset at the beginning of each loop iteration in the
for_each_set_bit(i, &ctr_mask, BITS_PER_LONG) loop, subsequent running counters
in the mask will be stopped, but the condition if (snap_flag_set && !sbiret)
will evaluate to false.

Does this cause successfully stopped counters to silently skip their snapshot
updates?

> -			if (pmc->cinfo.type == SBI_PMU_CTR_TYPE_FW)
> -				pmc->counter_val = kvpmu->fw_event[fevent_code].value;
> -			else if (pmc->perf_event)
> +			if (pmc->perf_event)
>  				pmc->counter_val += perf_event_read_value(pmc->perf_event,
>  									  &enabled, &running);
>  			/*

[Severity: Critical]
This is a pre-existing issue, but can the guest trigger a host kernel panic
here if it intentionally fails the shared memory setup?

If the guest calls SBI_EXT_PMU_SNAPSHOT_SET_SHMEM with a valid GPA,
kvpmu->snapshot_addr is set and kvpmu->sdata is allocated.

If the guest calls it again with an unmapped GPA:

arch/riscv/kvm/vcpu_pmu.c:kvm_riscv_vcpu_pmu_snapshot_set_shmem() {
    ...
    kvpmu->sdata = kzalloc(snapshot_area_size, GFP_ATOMIC);
    ...
    if (kvm_vcpu_write_guest(vcpu, shmem_base, kvpmu->sdata,
                             snapshot_area_size)) {
        kfree(kvpmu->sdata);
        kvpmu->sdata = NULL;
        sbiret = SBI_ERR_INVALID_ADDRESS;
        goto out;
    }
    ...
}

This unconditionally allocates a new kvpmu->sdata without freeing the old one,
causing a memory leak. Since kvm_vcpu_write_guest() fails, kvpmu->sdata is
freed and set to NULL, and the goto out bypasses updating kvpmu->snapshot_addr.

If the guest then calls SBI_EXT_PMU_COUNTER_STOP with
SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT, kvpmu->snapshot_addr is still the old valid
GPA. The stop operation bypasses the INVALID_GPA check in
kvm_riscv_vcpu_pmu_ctr_stop() and eventually executes:

        if (snap_flag_set && !sbiret) {
            ...
            kvpmu->sdata->ctr_values[i] = pmc->counter_val;

Will this dereference the NULL kvpmu->sdata pointer and cause a panic?

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