Re: [PATCH v2 1/3] KVM: PPC: Book3S HV: Maintain aggregate context switch values for vCPUs

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

New issues:
- [High] The patch breaks task-bound VPA PMU events by subtracting independent VCPU aggregates, leading to severe underflows.

Pre-existing issues:
- [Critical] Pre-existing Use-After-Free (UAF) and cross-VM data leak via stale `kvm_hstate.kvm_vcpu` pointer in VPA PMU callbacks.
--

--- Patch [1]: [PATCH v2 1/3] KVM: PPC: Book3S HV: Maintain aggregate context switch values for vCPUs ---
commit b8e49dde6d70a4c423cf40d03275e97149e3916e
Author: Gautam Menghani <[email protected]>

KVM: PPC: Book3S HV: Maintain aggregate context switch values for vCPUs

This commit changes the vCPU context switch latency variables in vcpu->arch
to store aggregate values rather than the latest snapshots read from PACA.
The goal is to provide each vCPU with its own record of total context
switch latencies for the vpa-pmu driver.

> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 61dbeea317f3b..342168b8bfc86 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -4163,12 +4163,12 @@ static void do_trace_nested_cs_time(struct kvm_vcpu *vcpu)
>  	trace_kvmppc_vcpu_stats(vcpu, l1_to_l2_ns - *l1_to_l2_cs_ptr,
>  					l2_to_l1_ns - *l2_to_l1_cs_ptr,
>  					l2_runtime_ns - *l2_runtime_agg_ptr);
> +	vcpu->arch.l1_to_l2_cs += (l1_to_l2_ns - *l1_to_l2_cs_ptr);
> +	vcpu->arch.l2_to_l1_cs += (l2_to_l1_ns - *l2_to_l1_cs_ptr);
> +	vcpu->arch.l2_runtime_agg += (l2_runtime_ns - *l2_runtime_agg_ptr);

[Severity: Critical]
This is a pre-existing issue, but does relying on kvm_hstate.kvm_vcpu expose
a use-after-free and cross-VM data leak here?

When a task-bound VPA PMU event schedules in, vpa_pmu_add() calls into KVM
which reads the vCPU from the physical CPU's PACA:

arch/powerpc/kvm/book3s_hv.c:kvmhv_get_l1_to_l2_cs_time_vcpu() {
    ...
    vcpu = local_paca->kvm_hstate.kvm_vcpu;
    if (vcpu) {
        arch = &vcpu->arch;
        return arch->l1_to_l2_cs;
    ...
}

Since local_paca->kvm_hstate.kvm_vcpu is set during guest entry in
kvmhv_run_single_vcpu() but never cleared on exit, it remains pointing to
the last vCPU that executed on this physical CPU.

If that vCPU belonged to a different VM, its context switch data is leaked.
If the VM was destroyed, wouldn't this dereference a freed vCPU pointer?

[Severity: High]
Will changing these fields to accumulate independent aggregates cause massive
underflows in the PMU driver?

Before this patch, these fields stored a snapshot of the global monotonic
timer. Now they store independent per-vCPU aggregates.

Because of the stale local_paca pointer issue mentioned above, vpa_pmu_add()
records the previous vCPU's aggregate as the starting baseline:

arch/powerpc/perf/vpa-pmu.c:vpa_pmu_add() {
    ...
    data = get_counter_data(event);
    local64_set(&event->hw.prev_count, data);
    ...
}

Later, vpa_pmu_read() subtracts this from the current vCPU's aggregate:

arch/powerpc/perf/vpa-pmu.c:vpa_pmu_read() {
    ...
    final_data = new_data - prev_data;
    local64_add(final_data, &event->count);
}

Since new_data and prev_data are now completely unrelated aggregates from
different vCPUs rather than two points on a shared global timeline, won't
this subtraction produce garbage values and severe underflows?

>  	*l1_to_l2_cs_ptr = l1_to_l2_ns;
>  	*l2_to_l1_cs_ptr = l2_to_l1_ns;
>  	*l2_runtime_agg_ptr = l2_runtime_ns;
> -	vcpu->arch.l1_to_l2_cs = l1_to_l2_ns;
> -	vcpu->arch.l2_to_l1_cs = l2_to_l1_ns;
> -	vcpu->arch.l2_runtime_agg = l2_runtime_ns;
>  }

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