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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v6 3/9] x86/hvm: block speculative out-of-bound accesses

>>> On 15.02.19 at 11:50, <nmanthey@xxxxxxxxx> wrote:
> On 2/15/19 09:55, Jan Beulich wrote:
>>>>> On 15.02.19 at 09:05, <nmanthey@xxxxxxxxx> wrote:
>>> On 2/12/19 15:14, Jan Beulich wrote:
>>>>>>> On 12.02.19 at 15:05, <nmanthey@xxxxxxxxx> wrote:
>>>>> On 2/12/19 14:25, Jan Beulich wrote:
>>>>>>>>> On 08.02.19 at 14:44, <nmanthey@xxxxxxxxx> wrote:
>>>>>>> @@ -4104,6 +4108,12 @@ static int hvmop_set_param(
>>>>>>>      if ( a.index >= HVM_NR_PARAMS )
>>>>>>>          return -EINVAL;
>>>>>>> +    /*
>>>>>>> +     * Make sure the guest controlled value a.index is bounded even 
>>>>>>> during
>>>>>>> +     * speculative execution.
>>>>>>> +     */
>>>>>>> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
>>>>>>> +
>>>>>>>      d = rcu_lock_domain_by_any_id(a.domid);
>>>>>>>      if ( d == NULL )
>>>>>>>          return -ESRCH;
>>>>>>> @@ -4370,6 +4380,12 @@ static int hvmop_get_param(
>>>>>>>      if ( a.index >= HVM_NR_PARAMS )
>>>>>>>          return -EINVAL;
>>>>>>> +    /*
>>>>>>> +     * Make sure the guest controlled value a.index is bounded even 
>>>>>>> during
>>>>>>> +     * speculative execution.
>>>>>>> +     */
>>>>>>> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
>>>>>> ... the usefulness of these two. To make forward progress it may
>>>>>> be worthwhile to split off these two changes into a separate patch.
>>>>>> If you're fine with this, I could strip these two before committing,
>>>>>> in which case the remaining change is
>>>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> Taking apart the commit is fine with me. I will submit a follow up
>>>>> change that does not update the values but fixes the reads.
>>>> As pointed out during the v5 discussion, I'm unconvinced that if
>>>> you do so the compiler can't re-introduce the issue via CSE. I'd
>>>> really like a reliable solution to be determined first.
>>> I cannot give a guarantee what future compilers might do. Furthermore, I
>>> do not want to wait until all/most compilers ship with such a
>>> controllable guarantee.
>> Guarantee? Future compilers are (hopefully) going to get better at
>> optimizing, and hence are (again hopefully) going to find more
>> opportunities for CSE. So the problem is going to get worse rather
>> than better, and the changes you're proposing to re-instate are
>> therefore more like false promises.
> I do not want to dive into compilers future here. I would like to fix
> the issue for todays compilers now and not wait until compilers evolved
> one way or another. For this patch, the relevant information is whether
> it should go in like this, or whether you want me to protect all the
> reads instead. Is there more data I shall provide to help make this
> decision?

I understand that you're not happy with what I've said, and you're
unlikely to become any happier with what I'll add. But please
understand that _if_ we make any changes to address issues with
speculation, the goal has to be that we don't have to come back
an re-investigate after every new compiler release.

Even beyond that - if, as you say, we'd limit ourselves to current
compilers, did you check that all of them at any optimization level
or with any other flags passed which may affect code generation
produce non-vulnerable code? And in particular considering the
case here never recognize CSE potential where we would like them
not to?

A code change is, imo, not even worthwhile considering to be put
in if it is solely based on the observations made with a limited set
of compilers and/or options. This might indeed help you, if you
care only about one specific environment. But by putting this in
(and perhaps even backporting it) we're sort of stating that the
issue is under control (to the best of our abilities, and for the given
area of code). For everyone.

So, to answer your question: From what we know, we simply
can't take a decision, at least not between the two proposed
variants of how to change the code. If there was a variant that
firmly worked, then there would not even be a need for any
discussion. And again from what we know, there is one
requirement that need to be fulfilled for a change to be
considered "firmly working": The index needs to be in a register.
There must not be a way for the compiler to undermine this,
be it by CSE or any other means.

Considering changes done elsewhere, of course this may be
taken with a grain of salt. In other places we also expect the
compiler to not emit unreasonable code (e.g. needlessly
spilling registers to memory just to then reload them). But
while that's (imo) a fine expectation to have when an array
index is used just once, it is unavoidably more complicated in
the case here as well as in the grant table one.


Xen-devel mailing list



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