Re: [Xen-devel] [PATCH SpectreV1+L1TF v4 09/11] x86/vioapic: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.01.19 at 12:03, <nmanthey@xxxxxxxxx> wrote:
> On 1/25/19 17:34, Jan Beulich wrote:
>>>>> On 23.01.19 at 12:57, <nmanthey@xxxxxxxxx> wrote:
>>> @@ -212,7 +217,12 @@ static void vioapic_write_redirent(
>>>      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>>>      union vioapic_redir_entry *pent, ent;
>>>      int unmasked = 0;
>>> -    unsigned int gsi = vioapic->base_gsi + idx;
>>> +    unsigned int gsi;
>>> +
>>> +    /* Make sure no out-of-bound value for idx can be used */
>>> +    idx = array_index_nospec(idx, vioapic->nr_pins);
>>> +
>>> +    gsi = vioapic->base_gsi + idx;
>> I dislike the disconnect from the respective bounds check: There's
>> only one caller, so the construct could be moved there, or
>> otherwise I'd like to see an ASSERT() added documenting that the
>> bounds check is expected to have happened in the caller.
> I agree that the idx value is used as an array index in this function
> only once. However, the gsi value also uses the value of idx, and as
> that is passed to other functions, I want to bound the gsi variable as
> well. Therefore, I chose to have a separate assignment for the idx variable.

I don't mind the separate assignment, and I didn't complain
about idx being used just once. What I said is that there's
only one caller of the function. If the bounds checking was
done there, "gsi" here would be equally "bounded" afaict.
And I did suggest an alternative in case you dislike the
moving of the construct you add.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel