[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 09.02.2021 14:56, Norbert Manthey wrote:
> 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.

Oh, I see. What I'd like to be the result of your re-arrangement is
symmetry between "get" and "set" where possible in this regard, and
asymmetry having a clear reason. Seeing that hvm_{get,set}_param()
are the non-static functions here, I think it would be quite
desirable for the bounds checking to live there. Since
hvm_allow_{get,set}_param() are specifically helpers of the two
earlier named functions, checks consistently living there would as
well be fine with me.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.