On 2/18/19 17:08, Jan Beulich wrote: >>>> On 18.02.19 at 14:49, <nmant...@amazon.de> wrote: >> On 2/15/19 11:34, Jan Beulich wrote: >>>>>> On 15.02.19 at 10:55, <nmant...@amazon.de> wrote: >>>> On 2/13/19 12:50, Jan Beulich wrote: >>>>>>>> On 08.02.19 at 14:44, <nmant...@amazon.de> wrote: >>>>>> Guests can issue grant table operations and provide guest controlled >>>>>> data to them. This data is also used for memory loads. To avoid >>>>>> speculative out-of-bound accesses, we use the array_index_nospec macro >>>>>> where applicable. However, there are also memory accesses that cannot >>>>>> be protected by a single array protection, or multiple accesses in a >>>>>> row. To protect these, a nospec barrier is placed between the actual >>>>>> range check and the access via the block_speculation macro. >>>>>> >>>>>> As different versions of grant tables use structures of different size, >>>>>> and the status is encoded in an array for version 2, speculative >>>>>> execution might touch zero-initialized structures of version 2 while >>>>>> the table is actually using version 1. >>>>> Why zero-initialized? Did I still not succeed demonstrating to you >>>>> that speculation along a v2 path can actually overrun v1 arrays, >>>>> not just access parts with may still be zero-initialized? >>>> I believe a speculative v2 access can touch data that has been written >>>> by valid v1 accesses before, zero initialized data, or touch the NULL >>>> page. Given the macros for the access I do not believe that a v2 access >>>> can touch a page that is located behind a page holding valid v1 data. >>> I've given examples before of how I see this to be possible. Would >>> you mind going back to one of the instances, and explaining to me >>> how you do _not_ see any room for an overrun there? Having >>> given examples, I simply don't know how else I can explain this to >>> you without knowing at what specific part of the explanation we >>> diverge. (And no, I'm not excluding that I'm making up an issue >>> where there is none.) >> What we want to real out is that the actually use version1, while >> speculation might use version2, right? I hope you refer to this example >> of your earlier email. >> >> On 1/29/19 16:11, Jan Beulich wrote: >>> 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). >> From how I read your example and my explanation, the key difference is >> in the size of the shared_raw array. In case gref 256 is a valid v1 >> handle, then the shared_raw array has space for at least 256 entries, as >> shared_raw was allocated for the number of requested entries. The access >> to shared_raw is controlled with the macro shared_entry_v2: >> 222 #define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t)) >> 223 #define shared_entry_v2(t, e) \ >> 224 ((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2]) >> Since the direct access to the shared_v2 array depends on the >> SHGNT_PER_PAGE_V2 value, this has to be less than the size of that >> array. Hence, shared_raw will not be overrun (neither for version 1 nor >> version 2). However, this division might result in accessing an element >> of shared_raw that has not been initialized by version1 before. However, >> right after allocation, shared_raw is zero initialized. Hence, this >> might result in an access of the NULL page. > The question is: How much of shared_raw[] will be zero-initialized? > The example I've given uses relatively small grant reference values, > so for the purpose here let's assume gt->max_grant_frames is 1. > In this case shared_raw[] is exactly one entry in size. Hence the > speculative access you describe will not necessarily access the NULL > page. > > Obviously the same issue exists with higher limits and higher grant > reference numbers. The solution to this problem is really simple, I mixed up grant frames and grant entries. I agree that shared_raw can be accessed out-of-bounds and should be protected. I will adapt the commit message accordingly, and revise the modifications I added to the code base. > >>>>>> @@ -1321,7 +1327,8 @@ unmap_common( >>>>>> goto unlock_out; >>>>>> } >>>>>> >>>>>> - act = active_entry_acquire(rgt, op->ref); >>>>>> + act = active_entry_acquire(rgt, array_index_nospec(op->ref, >>>>>> + >>>> nr_grant_entries(rgt))); >>>>> ... you add a use e.g. here to _guard_ against speculation. >>>> The adjustment you propose is to exchange the switch statement in >>>> nr_grant_entries with a if( evaluate_nospec( gt->gt_version == 1 ), so >>>> that the returned values are not speculated? >>> At this point I'm not proposing a particular solution. I'm just >>> putting on the table an issue left un-addressed. I certainly >>> wouldn't welcome converting the switch() to an if(), even if >>> right now there's no v3 on the horizon. (It's actually quite >>> the inverse: If someone came and submitted a patch to change >>> the various if()-s on gt_version to switch()-es, I'd welcome this.) >> I am happy to add block_speculation() macros into each case of the >> switch statement. > Ugly, but perhaps the only possible solution at this point. > >>>> Do you want me to >>>> cache the value in functions that call this method regularly to avoid >>>> the penalty of the introduced lfence for each call? >>> That would go back to the question of what good it does to >>> latch value into a local variable when you don't know whether >>> the compiler will put that variable in a register or in memory. >>> IOW I'm afraid that to be on the safe side there's no way >>> around the repeated LFENCEs. >> The difference here would be that in case the value is stored into a >> local variable (independently of memory or register) and an lfence was >> executed, this value can be trusted and does not have to be checked >> again, as it's no longer guest controlled. > Ah, yes, you're right (it just wasn't clear to me that you implied > adding a fence together with the caching of the value). So perhaps > that's then also the way to go for the hunks under discussion in > patch 3?
Well, yes. That should effectively bound the a.index values in the other hunks in patch 3 as well. I will adapt that patch accordingly. Until now, I stepped back from this solution, as I want to avoid using the lfence instruction as much as possible. 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
