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

[Norbert Manthey <nmanthey@amazon.de>]

On 2/15/19 12:46, Jan Beulich wrote:
>>>> On 15.02.19 at 11:50, <nmanthey@amazon.de> wrote:
>> On 2/15/19 09:55, Jan Beulich wrote:
>>>>>> On 15.02.19 at 09:05, <nmanthey@amazon.de> wrote:
>>>> On 2/12/19 15:14, Jan Beulich wrote:
>>>>>>>> On 12.02.19 at 15:05, <nmanthey@amazon.de> wrote:
>>>>>> On 2/12/19 14:25, Jan Beulich wrote:
>>>>>>>>>> On 08.02.19 at 14:44, <nmanthey@amazon.de> 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@suse.com>
>>>>>> 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.
I do not see how a fix for problems like the discussed one could enter
the code base given the above conditions. However, for this very
specific fix, there fortunately is a comparison wrt a constant, and
there are many instructions until the potential speculative out-of-bound
access might happen, so that not fixing the two above access is fine for
me. While I cannot guarantee that it is not possible, we did not manage
to come up with a PoC for these two places with the effort we put into this.
> 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.

Unless you outline a path forward to fix the above two gadgets, I will
not include the above hunks in the next version of the series.

Best,
Norbert





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel