>>> On 20.05.19 at 16:27, <nmant...@amazon.de> wrote:
> On 3/29/19 18:11, Jan Beulich wrote:
>>>>> On 14.03.19 at 13:50, <nmant...@amazon.de> wrote:
>>> @@ -2410,9 +2448,11 @@ acquire_grant_for_copy(
>>> PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
>>> "Bad grant reference %#x\n", gref);
>>>
>>> - act = active_entry_acquire(rgt, gref);
>>> + /* This call ensures the above check cannot be bypassed speculatively
>>> */
>>> shah = shared_entry_header(rgt, gref);
>>> - if ( rgt->gt_version == 1 )
>>> + act = active_entry_acquire(rgt, gref);
>>> +
>>> + if ( evaluate_nospec(rgt->gt_version == 1) )
>>> {
>>> sha2 = NULL;
>>> status = &shah->flags;
>> What about the second version check further down in this function?
> That one should be fine, as it exists that function afterwards, and
> represents an abort path that is valid for both versions.
That's not obvious from looking at just the if() in question. For
example, fixup_status_for_copy_pin() gets handed "status" as
an argument, which is version dependent. It seems quite likely
indeed that no code changes are here, but this imo again needs
discussing/explaining in the commit message.
>>> @@ -3838,6 +3886,9 @@ static int gnttab_get_status_frame_mfn(struct domain
>>> *d,
>>> return -EINVAL;
>>> }
>>>
>>> + /* Make sure idx is bounded wrt nr_status_frames */
>>> + block_speculation();
>>> +
>>> *mfn = _mfn(virt_to_mfn(gt->status[idx]));
>> Why not array_access_nospec()? And how is this different from
>> gnttab_get_shared_frame_mfn(), which you don't change?
>
> This idx access is to a version dependent structure, and hence
> array_index_nospec is not good enough to catch the version difference
> case as well.
But the comment talks about the array bound only.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
