Re: [PATCH v3] KVM: x86: use array_index_nospec with indices that come from guest

[Sean Christopherson <seanjc@google.com>]

On Thu, Mar 05, 2026, David Woodhouse wrote:
> On 5 March 2026 23:29:11 CET, Sean Christopherson <seanjc@google.com> wrote:
> >On Thu, Mar 05, 2026, Jim Mattson wrote:
> >> On Thu, Mar 5, 2026 at 12:31 PM David Woodhouse <dwmw2@infradead.org> wrote:
> >> >
> >> > On Mon, 2025-08-04 at 08:44 +0200, Thijs Raymakers wrote:
> >> > > min and dest_id are guest-controlled indices. Using array_index_nospec()
> >> > > after the bounds checks clamps these values to mitigate speculative execution
> >> > > side-channels.
> >> > >
> >> >
> >> > (commit c87bd4dd43a6)
> >> >
> >> > Is this sufficient in the __pv_send_ipi() case?
> >> >
> >> > > --- a/arch/x86/kvm/lapic.c
> >> > > +++ b/arch/x86/kvm/lapic.c
> >> > > @@ -852,6 +852,8 @@ static int __pv_send_ipi(unsigned long *ipi_bitmap, struct kvm_apic_map *map,
> >> > >       if (min > map->max_apic_id)
> >> > >               return 0;
> >> > >
> >> > > +     min = array_index_nospec(min, map->max_apic_id + 1);
> >> > > +
> >> > >       for_each_set_bit(i, ipi_bitmap,
> >> > >               min((u32)BITS_PER_LONG, (map->max_apic_id - min + 1))) {
> >> > >               if (map->phys_map[min + i]) {
> >> >                         vcpu = map->phys_map[min + i]->vcpu;
> >> >                         count += kvm_apic_set_irq(vcpu, irq, NULL);
> >> >                 }
> >> >         }
> >> >
> >> > Do we need to protect [min + i] in the loop, rather than just [min]?
> >> >
> >> > The end condition for the for_each_set_bit() loop does mean that it
> >> > won't actually execute past max_apic_id but is that sufficient to
> >> > protect against *speculative* execution?
> >> >
> >> > I have a variant of this which uses array_index_nospec(min+i, ...)
> >> > *inside* the loop.
> >> 
> >> Heh. Me too!
> >
> >LOL, OMG, get off your high horses you two and someone send a damn patch!  
> 
> Heh, happy to, but it was actually a genuine question. Our pre-embargo
> patches did it in the loop but the most likely explanation seemed to be that
> upstream changed it as a valid optimization (because somehow the loop wasn't
> vulnerable?), and that we *can* drop the old patches in favour of the
> upstream one.
> 
> If no such reason exists for why the patch got changed, I'm happy to post the
> delta.

AFAIK, there was no such justification.  I'm pretty sure the only upstream version
I've ever seen is what ended up in-tree.

Speculation stuff definitely isn't my area of expertise.  Honestly, you, Jim, and
a few others are who I'd go bug for answers for this sort of thing, so unless
someone chimes in with a strong argument for the current code, I say we go with
the more conservative approach.