On 2/15/19 12:46, Jan Beulich wrote: >>>> On 15.02.19 at 11:50, <nmant...@amazon.de> wrote: >> On 2/15/19 09:55, Jan Beulich wrote: >>>>>> On 15.02.19 at 09:05, <nmant...@amazon.de> wrote: >>>> On 2/12/19 15:14, Jan Beulich wrote: >>>>>>>> On 12.02.19 at 15:05, <nmant...@amazon.de> wrote: >>>>>> On 2/12/19 14:25, Jan Beulich wrote: >>>>>>>>>> On 08.02.19 at 14:44, <nmant...@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 <jbeul...@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
