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

On Thu, Aug 13, 2026 at 10:03:04AM +0000, [email protected] wrote:
> 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?
> 

This is a valid find - kvmhv_run_single_vcpu() is called after
vpa_pmu_add() is called, so the paca stores a stale 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;
> >  }
> 

Yes, this is a consequence of the bug reported above.

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