On 1/29/19 16:11, Jan Beulich wrote: >>>> On 29.01.19 at 14:47, <nmant...@amazon.de> wrote: >> On 1/29/19 10:46, Jan Beulich wrote: >>>>>> Norbert Manthey <nmant...@amazon.de> 01/29/19 9:35 AM >>> >>>> I am aware that both version use the same base array, and access it via >>>> different macros, which essentially partition the array based on the >>>> size of the respective struct. The underlying raw array has the same >>>> size for both version. >>> And this is the problem afaics: If a guest has requested its grant table to >>> be sized as a single page, this page can fit twice as many entries for >>> v1 than it can fit for v2. Hence the v1 grant reference pointing at the last >>> entry would point at the last entry in the (not mapped) second page for v2. >> I might understand the code wrong, but a guest would ask to get at most >> N grant frames, and this number cannot be increased afterwards, i.e. the >> field gt->max_grant_frames is written exactly once. Furthermore, the >> void** shared_raw array is allocated and written exactly once with >> sufficient pointers for, namely gt->max_grant_frames many in function >> grant_table_init. Hence, independently of the version being used, at >> least the shared_raw array cannot be used for out-of-bound accesses >> during speculation with my above evaluate_nospec. > I'm afraid I'm still not following: A give number of pages is worth > twice as many grants in v1 than it is in v2. Therefore a v1 grant > reference to a grant entry tracked in the second half of the > first page would cause a speculative access to anywhere in the > second page when wrongly interpreted as a v2 ref. Agreed. So you want me to add another lfence to make sure the wrong interpretation does not lead to other out-of-bound accesses down the speculative window? In my opinion, the v1 vs v2 code does not result in actual out-of-bound accesses, except for the NULL page case below. To make the PV case happy, I will add the evaluate_nospec macro for the v1 vs v2 conditionals in functions with guest controlled ref indexes. > >> That being said, let's assume we have a v1 grant table, and speculation >> uses the v2 accesses. In that case, an existing and zero-initialized >> entry of shared_raw might be used in the first part of the >> shared_entry_v2 macro, and even if that pointer would be non-NULL, the >> page it would point to would have been cleared when growing the grant >> table in function gnttab_grow_table. > Not if the v1 ref is no smaller than half the maximum number of > v1 refs. In that case, if taken as a v2 ref, ->shared_raw[] > would need to be twice as big to cope with the larger index > (resulting from the smaller divisor in shared_entry_v2() > compared to shared_entry_v1()) in order to not be overrun. > > Let's look at an example: gref 256 points into the middle of > the first page when using v1 calculations, but at the start > of the second page when using v2 calculations. Hence, if the > maximum number of grant frames was 1, we'd overrun the > array, consisting of just a single element (256 is valid as a > v1 gref in that case, but just out of bounds as a v2 one). If 256 is a valid gref, then the shared_raw array holds sufficient zero-initialized elements for such an access, even without the division operator that is used in the shared_entry_v*() macros. Hence, no out-of-bound access will happen here. > > Furthermore, even if ->shared_raw[] itself could not be overrun, > an entry of it being NULL could be a problem with PV guests, who > can install a translation for the first page of the address space, > and thus perhaps partly control subsequent speculative execution.
I understand the concern. I add the evaluate_nospec as mentioned above. Best, Norbert > > Jan > > 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
