Re: [PATCH HVM v2 1/1] hvm: refactor set param

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2021 14:41, Norbert Manthey wrote:
> On 2/9/21 10:40 AM, Jan Beulich wrote:
>> On 08.02.2021 20:47, Norbert Manthey wrote:
>>> On 2/8/21 3:21 PM, Jan Beulich wrote:
>>>> On 05.02.2021 21:39, Norbert Manthey wrote:
>>>>> @@ -4108,6 +4108,13 @@ static int hvm_allow_set_param(struct domain *d,
>>>>>      if ( rc )
>>>>>          return rc;
>>>>>
>>>>> +    if ( index >= HVM_NR_PARAMS )
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    /* Make sure we evaluate permissions before loading data of domains. 
>>>>> */
>>>>> +    block_speculation();
>>>>> +
>>>>> +    value = d->arch.hvm.params[index];
>>>>>      switch ( index )
>>>>>      {
>>>>>      /* The following parameters should only be changed once. */
>>>> I don't see the need for the heavier block_speculation() here;
>>>> afaict array_access_nospec() should do fine. The switch() in
>>>> context above as well as the switch() further down in the
>>>> function don't have any speculation susceptible code.
>>> The reason to block speculation instead of just using the hardened index
>>> access is to not allow to speculatively load data from another domain.
>> Okay, looks like I got mislead by the added bounds check. Why
>> do you add that, when the sole caller already has one? It'll
>> suffice since you move the array access past the barrier,
>> won't it?
> I can drop that bound check again. This was added to make sure other
> callers would be save as well. Thinking about this a little more, the
> check could actually be moved into the hvm_allow_set_param function,
> above the first index access in that function. Are there good reasons to
> not move the index check into the allow function?

I guess I'm confused: We're talking about dropping the check
you add to hvm_allow_set_param() and you suggest to "move" it
right there?

Jan