Re: [PATCH SpectreV1+L1TF v6 8/9] common/grant_table: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.02.19 at 10:55, <nmanthey@amazon.de> wrote:
> On 2/13/19 12:50, Jan Beulich wrote:
>>>>> On 08.02.19 at 14:44, <nmanthey@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.)

>>> @@ -963,9 +965,13 @@ map_grant_ref(
>>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>                   op->ref, rgt->domain->domain_id);
>>>  
>>> +    /* Make sure the above check is not bypassed speculatively */
>>> +    block_speculation();
>>> +
>>>      act = active_entry_acquire(rgt, op->ref);
>>>      shah = shared_entry_header(rgt, op->ref);
>>> -    status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, op->ref);
>>> +    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
>>> +                                                 : &status_entry(rgt, op->ref);
>> Did you consider folding the two pairs of fences you emit into
>> one? Moving up the assignment to status ought to achieve this,
>> as then the block_speculation() could be dropped afaict.
>>
>> Then again you don't alter shared_entry_header(). If there's
>> a reason for you not having done so, then a second fence
>> here is needed in any event.
> Instead of the block_speculation() macro, I can also protect the op->ref
> usage before evaluate_nospec via the array_index_nospec function.

That's an option (as before), but doesn't help with shared_entry_header()'s
evaluation of gt_version.

>> What about the version check in nr_grant_entries()? It appears
>> to me as if at least its use in grant_map_exists() (which simply is
>> the first one I've found) is problematic without an adjustment.
>> Even worse, ...
>>
>>> @@ -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.)

> Already before this
> modification the function is called and not inlined.

How does this matter when considering speculation?

> 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.

>> And what about _set_status(), unmap_common_complete(),
>> gnttab_grow_table(), gnttab_setup_table(),
>> release_grant_for_copy(), the 2nd one in acquire_grant_for_copy(),
>> several ones in gnttab_set_version(), gnttab_release_mappings(),
>> the 3rd one in mem_sharing_gref_to_gfn(), gnttab_map_frame(),
>> and gnttab_get_status_frame()?
> 
> Protecting the function itself should allow to not modify the
> speculation guards in these functions. I would have to check each of
> them, whether the guest actually has control, and whether it makes sense
> to introduce a _nospec variant of the nr_grant_entries function to not
> penalize everywhere. Do you have an opinion on this?

As per above, yes, I think the only sufficiently reliable way is
to modify nr_grant_entries() itself. The question is how to
correctly do this without replacing the switch() there, the more
that the other change of yours has deliberately forced the
compiler into using rows of compares instead of jump tables (not
that I'd expect the compiler to have used a jump table here, i.e.
the remark is just wrt the general case).

Jan


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