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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>>> @@ -4141,6 +4148,9 @@ static int hvm_set_param(struct domain *d, uint32_t 
>>> index, uint64_t value)
>>>      if ( rc )
>>>          return rc;
>>>
>>> +    /* Make sure we evaluate permissions before loading data of domains. */
>>> +    block_speculation();
>>> +
>>>      switch ( index )
>>>      {
>>>      case HVM_PARAM_CALLBACK_IRQ:
>> Like you do for the "get" path I think this similarly renders
>> pointless the use in hvmop_set_param() (and - see below - the
>> same consideration wrt is_hvm_domain() applies).
> Can you please be more specific why this is pointless? I understand that
> the is_hvm_domain check comes with a barrier that can be used to not add
> another barrier. However, I did not find such a barrier here, which
> comes between the 'if (rc)' just above, and the potential next access
> based on the value of 'index'. At least the access behind the switch
> statement cannot be optimized and replaced with a constant value easily.

I'm suspecting a misunderstanding (the more that further down
you did agree to what I've said for hvmop_get_param()): I'm
not saying your addition is pointless. Instead I'm saying that
your addition should be accompanied by removal of the barrier
from hvmop_set_param(), paralleling what you do to
hvmop_get_param(). And additionally I'm saying that just like
in hvmop_get_param() the barrier there was already previously
redundant with that inside is_hvm_domain().

Jan