Re: [Xen-devel] [PATCH L1TF MDS GT v1 3/3] common/grant_table: harden version dependent accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.05.19 at 09:45, <nmanthey@xxxxxxxxx> wrote:
> Guests can issue grant table operations and provide guest controlled
> data to them. This data is used as index for memory loads after bound
> checks have been done. Depending on the grant table version, the
> size of elements in containers differ. As the base data structure is
> a page, the number of elements per page also differs. Consequently,
> bound checks are version dependent, so that speculative execution can
> happen in several stages, the bound check as well as the version check.
> 
> This commit mitigates cases where out-of-bound accesses could happen
> due to the version comparison. In cases, where no different memory
> locations are accessed on the code path that follow an if statement,
> no protection is required. No different memory locations are accessed
> in the following functions after a version check:
> 
>  * gnttab_setup_table: only calculated numbersi are used, and then
>         function gnttab_grow_table is called, which is version protected
> 
>  * gnttab_transfer: the case that depends on the version check just gets
>         into copying a page or not

Well, this is a little lax, but I'm willing to accept it. There could, after
all, still be speculation into alloc_domheap_page().

>  * acquire_grant_for_copy: the not fixed comparison is on the abort path
>         and does not access other structures, and on the else branch only
>         accesses structures that are properly allocated

As said in my recent reply to v10 of the original series, in particular
for fixup_status_for_copy_pin() this isn't immediately obvious. In
no case is "does not access other structures" true, though. How
about saying "accesses only structures that have been validated
before" or some such instead (I don't particularly like "validated"
here, but I can't currently think of anything better)?

>  * gnttab_set_version: all accessible data is allocated for both versions

This is not enough for my taste: The very first loop is safe only
because nr_grant_entries() is. And speculating into
gnttab_unpopulate_status_frames() doesn't look safe at all, as
gt->status[i] may be NULL.

>  * gnttab_release_mappings: this function is called only during domain
>        destruction and control is not returned to the guest
> 
>  * mem_sharing_gref_to_gfn: speculation will be stoped by the second if
>        statement, as that places a barrier on any path to be executed.
> 
>  * gnttab_get_status_frame_mfn: no version dependent check, because all
>        accesses, except the gt->status[idx], do not perform actual
>        out-of-bound accesses, including the gnttab_grow_table function
>        call.

Nit: I very much hope no code anywhere performs _actual_ out of
bound accesses. I'm sure you mean speculative ones here.

>  * gnttab_get_shared_frame: block_speculation in
>        gnttab_get_status_frame_mfn blocks accesses

The former doesn't call the latter, and as per my patch 2 comments
gnttab_get_shared_frame_mfn() looks to remain unprotected after
patch 2.

Jan



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