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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 2/9/21 2:45 PM, Jan Beulich wrote:
> 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?

Yes. I can either just no introduce that check in that function (which
is what you suggested). As an alternative, to have all checks in one
function, I proposed to instead move it into hvm_allow_set_param. If you
do not like this proposal, I'll follow your suggestion and will simply
not introduce the additional bound check as part of this patch.

Best,
Norbert




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879