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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 1/28/19 12:12, Jan Beulich wrote:
>>>> 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.

Ah, I understand your previous sentence differently now. Thanks for
clarifying. I like to keep the nospec statements close to the
problematic use, so that eventual future callers benefit from that as
well. Therefore, I'll add an ASSERT statement with the bound check.

Best,
Norbert

>
> Jan
>
>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

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