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

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

[Thijs Raymakers]

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.

Signed-off-by: Thijs Raymakers <thijs@raymakers.nl>
Cc: stable <stable@kernel.org>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Sean C. correctly pointed out that max_apic_id is inclusive, while
array_index_nospec is not.

Changes in v3:
- Put the diff and the changes to the feedback in a single patch, instead
  of spreading it out over multiple emails.
- Remove premature SoB.
- Not sent as a In-Reply-To the previous version.

Changes in v2:
- Add one to the length argument of array_index_nospec, so it becomes
  inclusive with max_apic_id.

---
 arch/x86/kvm/lapic.c | 2 ++
 arch/x86/kvm/x86.c   | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 73418dc0ebb2..0725d2cae742 100644
--- 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]) {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 93636f77c42d..43b63f1d1594 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10051,8 +10051,11 @@ static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
 	rcu_read_lock();
 	map = rcu_dereference(vcpu->kvm->arch.apic_map);
 
-	if (likely(map) && dest_id <= map->max_apic_id && map->phys_map[dest_id])
-		target = map->phys_map[dest_id]->vcpu;
+	if (likely(map) && dest_id <= map->max_apic_id) {
+		dest_id = array_index_nospec(dest_id, map->max_apic_id + 1);
+		if (map->phys_map[dest_id])
+			target = map->phys_map[dest_id]->vcpu;
+	}
 
 	rcu_read_unlock();
 
-- 
2.50.1

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

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.
> 
> Signed-off-by: Thijs Raymakers <thijs@raymakers.nl>
> Cc: stable <stable@kernel.org>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> Sean C. correctly pointed out that max_apic_id is inclusive, while
> array_index_nospec is not.

Fixes: 715062970f37 ("KVM: X86: Implement PV sched yield hypercall")

?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

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

[Jim Mattson]

On Tue, Aug 12, 2025 at 3:37 AM 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.
> >
> > Signed-off-by: Thijs Raymakers <thijs@raymakers.nl>
> > Cc: stable <stable@kernel.org>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > Sean C. correctly pointed out that max_apic_id is inclusive, while
> > array_index_nospec is not.
>
> Fixes: 715062970f37 ("KVM: X86: Implement PV sched yield hypercall")

And possibly:

Fixes: bdf7ffc89922 ("KVM: LAPIC: Fix pv ipis out-of-bounds access")

Though, perhaps the blame really lies with commit 4180bf1b655a ("KVM:
X86: Implement "send IPI" hypercall").


> ?

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

[Sean Christopherson]

On Mon, 04 Aug 2025 08:44:05 +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.

Finally applied to kvm-x86 fixes, thanks!

[1/1] KVM: x86: use array_index_nospec with indices that come from guest
      https://github.com/kvm-x86/linux/commit/c87bd4dd43a6

--
https://github.com/kvm-x86/linux/tree/next

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

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.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

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

[Jim Mattson]

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!

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

[Sean Christopherson]

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!  

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

[David Woodhouse]

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.

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

[Sean Christopherson]

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.

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

On Thu, 2026-03-05 at 17:54 -0800, Sean Christopherson wrote:
> 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.

Posted as
https://lore.kernel.org/kvm/9d50fc3ca9e8e58f551d015f95d51a3c29ce6ccc.camel@infradead.org


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

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

[Thijs Raymakers]

On 3/6/26 9:05 AM, David Woodhouse wrote:
> On Thu, 2026-03-05 at 17:54 -0800, Sean Christopherson wrote:
>> 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.
> Posted as
> https://lore.kernel.org/kvm/9d50fc3ca9e8e58f551d015f95d51a3c29ce6ccc.camel@infradead.org

I looked back to my notes, and this seems to be an oversight on my end. 
The goal of the original
patch was to remove a speculative cache load with an arbitrary value of 
'min', because it allowed
a guest to load any host memory into its cache. I thought it reduced the 
maximum speculative buffer
out-of-bounds load from all of host memory to no out-of-bounds load, but 
it reduced it to a maximum
of BITS_PER_LONG elements of map->phys_map instead.

For the perspective of my original proof-of-concept, I do not see how an 
attacker could abuse this
small window. In theory, depending on the data that sits directly after 
the phys_map array, it could
theory be used to leak a pointer from the host (and probably not much 
else). Last time I checked, there
was no data directly after the phys_map array, but that could change due 
to any number of reasons
that are unrelated to this particular piece of code.

I am in favor of the more conservative approach and using 
array_index_nospec *inside* of the loop.
The original patch does prevent my original proof-of-concept, which is 
probably why I missed this. I
do not think that the current speculative out-of-bounds load is 
exploitable in practice, but it is better
to take a more conservative approach in this case.

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

[Paolo Bonzini]

Il gio 5 mar 2026, 21:31 David Woodhouse <dwmw2@infradead.org> ha scritto:
>
> 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?

You would be able to load extra values in the cache but it would not
be a fully guest controlled load in the same way that Spectre wants to
do it. Spectre works because the value used in the speculative load is
way beyond the maximum and points to attacker-controlled memory.

That said it doesn't hurt either, other than a few clock cycles for
not hoisting array_index_nospec out of the loop.

Paolo

>
> I have a variant of this which uses array_index_nospec(min+i, ...)
> *inside* the loop.
>