Re: [PATCH v1 2/3] RISC-V: KVM: Report snapshot write failure to the guest

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

Pre-existing issues:
- [Critical] Repeated or failed calls to SBI_EXT_PMU_SNAPSHOT_SET_SHMEM cause a memory leak and lead to a NULL pointer dereference.
- [Critical] Integer overflow in kvm_riscv_vcpu_pmu_event_info causes a massive heap buffer overflow.
- [High] A failure while stopping one PMU counter prevents snapshot updates for all subsequent successfully stopped counters.
- [High] Starting an already-started counter silently corrupts its base value and violates state atomicity.
--

--- Patch [2]: [PATCH v1 2/3] RISC-V: KVM: Report snapshot write failure to the guest ---
commit 2b7f150df4c06e5c182586d3b92e176b5a3ac870
Author: SeungJu Cheon <[email protected]>

RISC-V: KVM: Report snapshot write failure to the guest

If kvm_vcpu_write_guest() fails while updating the PMU snapshot area
on counter stop, the guest may receive SBI_SUCCESS without the
snapshot being updated, leaving stale data in shared memory. This
commit ensures SBI_ERR_FAILURE is returned when the write fails.

> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 56dd5b1b2846c..45dd5233340dc 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -723,9 +723,10 @@ int kvm_riscv_vcpu_pmu_ctr_stop(struct kvm_vcpu *vcpu, unsigned long ctr_base,
>  		}
>  	}
>  
> -	if (shmem_needs_update)
> -		kvm_vcpu_write_guest(vcpu, kvpmu->snapshot_addr, kvpmu->sdata,
> -					     sizeof(struct riscv_pmu_snapshot_data));
> +	if (shmem_needs_update &&
> +	    kvm_vcpu_write_guest(vcpu, kvpmu->snapshot_addr, kvpmu->sdata,
> +				 sizeof(struct riscv_pmu_snapshot_data)))
> +		sbiret = SBI_ERR_FAILURE;
>  
>  out:
>  	retdata->err_val = sbiret;

[Severity: High]
This isn't a bug introduced by this patch, but looking at the loop earlier in
kvm_riscv_vcpu_pmu_ctr_stop():

	for_each_set_bit(i, &ctr_mask, BITS_PER_LONG) {
		/* ... */
		if (snap_flag_set && !sbiret) {
			/* ... */
			kvpmu->sdata->ctr_values[i] = pmc->counter_val;
			shmem_needs_update = true;
		}

Does a failure while stopping one PMU counter prevent snapshot updates for all
subsequent successfully stopped counters? 

The loop relies on sbiret being zero to update the snapshot, but sbiret acts
as a global error flag that is never cleared between iterations.


[Severity: High]
This is also a pre-existing issue, but I noticed a potential concern in
kvm_riscv_vcpu_pmu_ctr_start():

	for_each_set_bit(i, &ctr_mask, BITS_PER_LONG) {
		/* ... */
		pmc = &kvpmu->pmc[pmc_index];
		if (flags & SBI_PMU_START_FLAG_SET_INIT_VALUE) {
			pmc->counter_val = ival;
		} /* ... */

		} else if (pmc->perf_event) {
			if (unlikely(pmc->started)) {
				sbiret = SBI_ERR_ALREADY_STARTED;
				continue;
			}

Does starting an already-started counter silently corrupt its base value?

The pmc->counter_val is modified before validating the pmc->started state,
which could permanently overwrite the active counter base value.


[Severity: Critical]
This is a pre-existing issue, but can repeated or failed calls to
SBI_EXT_PMU_SNAPSHOT_SET_SHMEM cause a memory leak and lead to a NULL
pointer dereference?

In kvm_riscv_vcpu_pmu_snapshot_set_shmem():

	kvpmu->sdata = kzalloc(snapshot_area_size, GFP_ATOMIC);
	if (!kvpmu->sdata) {
		sbiret = SBI_ERR_FAILURE;
		goto out;
	}

	/* No need to check writable slot explicitly as kvm_vcpu_write_guest does it internally */
	if (kvm_vcpu_write_guest(vcpu, saddr, kvpmu->sdata, snapshot_area_size)) {
		kfree(kvpmu->sdata);
		kvpmu->sdata = NULL;
		sbiret = SBI_ERR_INVALID_ADDRESS;
		goto out;
	}

It unconditionally overwrites kvpmu->sdata without freeing the previous
allocation. Additionally, upon error, it sets kvpmu->sdata to NULL without
clearing kvpmu->snapshot_addr.

Later, in kvm_riscv_vcpu_pmu_ctr_stop():

		if (snap_flag_set && !sbiret) {
			/* ... */
			if (test_bit(pmc_index, kvpmu->pmc_overflown))
				kvpmu->sdata->ctr_overflow_mask |= BIT(i);
			kvpmu->sdata->ctr_values[i] = pmc->counter_val;

The mismatch between a valid snapshot_addr and a NULL sdata could cause a
direct NULL pointer dereference here.


[Severity: Critical]
This isn't a bug introduced by this patch, but does the shmem_size calculation
in kvm_riscv_vcpu_pmu_event_info() overflow?

	struct riscv_pmu_event_info *einfo = NULL;
	int shmem_size = num_events * sizeof(*einfo);
	/* ... */
	einfo = kzalloc(shmem_size, GFP_KERNEL);
	/* ... */
	for (int i = 0; i < num_events; i++) {
		/* ... */
		einfo[i].output = (ret > 0) ? 1 : 0;
	}

The integer truncation of shmem_size results in a small allocation when
num_events is very large (e.g., 0x10000001), while the loop uses the
untruncated num_events. 

Can this cause out-of-bounds heap writes during the loop execution?

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