| Message ID | 1558424746-24059-4-git-send-email-nmanthey@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [L1TF,MDS,GT,v1,1/3] common/grant_table: harden helpers | expand |
>>> On 21.05.19 at 09:45, <nmanthey@amazon.de> 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
On 5/23/19 17:01, Jan Beulich wrote: >>>> On 21.05.19 at 09:45, <nmanthey@amazon.de> 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)? I will rephrase accordingly. > >> * 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. So, you basically want to see a block_speculation() at the beginning of the function gnttab_populate_status_frames and gnttab_unpopulate_status_frames? I do not claim to protect against speculative out-of-bound accesses that are caused by the for loop in gnttab_set_version. > >> * 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. Yes, I do not mean actual out-of-bound accesses. What I actually meant: all other accesses in this function are to variables, and not based on an index. > >> * 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. True, I will add a block_speculation() below the assert statement, so that the condition is true even when executing speculatively. Best, Norbert Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 08.07.2019 15:53, Norbert Manthey wrote: > On 5/23/19 17:01, Jan Beulich wrote: >>>>> On 21.05.19 at 09:45, <nmanthey@amazon.de> wrote: >>> * 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. > So, you basically want to see a block_speculation() at the beginning of > the function gnttab_populate_status_frames and > gnttab_unpopulate_status_frames? I do not claim to protect against > speculative out-of-bound accesses that are caused by the for loop in > gnttab_set_version. The point isn't the loop, but the fact that by mis-speculating through the two conditions before the function call a NULL gt->status[0] may get accessed, entirely independent of this being a loop or just a singular access. Jan
On 7/10/19 05:12, Jan Beulich wrote: > On 08.07.2019 15:53, Norbert Manthey wrote: >> On 5/23/19 17:01, Jan Beulich wrote: >>>>>> On 21.05.19 at 09:45, <nmanthey@amazon.de> wrote: >>>> * 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. >> So, you basically want to see a block_speculation() at the beginning of >> the function gnttab_populate_status_frames and >> gnttab_unpopulate_status_frames? I do not claim to protect against >> speculative out-of-bound accesses that are caused by the for loop in >> gnttab_set_version. > The point isn't the loop, but the fact that by mis-speculating through > the two conditions before the function call a NULL gt->status[0] may > get accessed, entirely independent of this being a loop or just a > singular access. I understand. To prevent this kind of access during speculative execution, I will add a block_speculation() at the top of the function to make sure the code is reached only when the correct version number is used. Best, Norbert Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -837,7 +837,7 @@ static int _set_status(unsigned gt_version, grant_status_t *status) { - if ( gt_version == 1 ) + if ( evaluate_nospec(gt_version == 1) ) return _set_status_v1(domid, readonly, mapflag, shah, act); else return _set_status_v2(domid, readonly, mapflag, shah, act, status); @@ -990,9 +990,12 @@ map_grant_ref( /* This call also ensures the above check cannot be passed speculatively */ shah = shared_entry_header(rgt, op->ref); - status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, op->ref); act = active_entry_acquire(rgt, op->ref); + /* Make sure we do not access memory speculatively */ + status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags + : &status_entry(rgt, op->ref); + /* If already pinned, check the active domid and avoid refcnt overflow. */ if ( act->pin && ((act->domid != ld->domain_id) || @@ -1013,7 +1016,7 @@ map_grant_ref( if ( !act->pin ) { - unsigned long gfn = rgt->gt_version == 1 ? + unsigned long gfn = evaluate_nospec(rgt->gt_version == 1) ? shared_entry_v1(rgt, op->ref).frame : shared_entry_v2(rgt, op->ref).full_page.frame; @@ -1463,7 +1466,7 @@ unmap_common_complete(struct gnttab_unmap_common *op) act = active_entry_acquire(rgt, op->ref); sha = shared_entry_header(rgt, op->ref); - if ( rgt->gt_version == 1 ) + if ( evaluate_nospec(rgt->gt_version == 1) ) status = &sha->flags; else status = &status_entry(rgt, op->ref); @@ -1795,7 +1798,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames) } /* Status pages - version 2 */ - if ( gt->gt_version > 1 ) + if ( evaluate_nospec(gt->gt_version > 1) ) { if ( gnttab_populate_status_frames(d, gt, req_nr_frames) ) goto shared_alloc_failed; @@ -2290,7 +2293,7 @@ gnttab_transfer( grant_read_lock(e->grant_table); act = active_entry_acquire(e->grant_table, gop.ref); - if ( e->grant_table->gt_version == 1 ) + if ( evaluate_nospec(e->grant_table->gt_version == 1) ) { grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref); @@ -2351,7 +2354,7 @@ release_grant_for_copy( sha = shared_entry_header(rgt, gref); mfn = act->mfn; - if ( rgt->gt_version == 1 ) + if ( evaluate_nospec(rgt->gt_version == 1) ) { status = &sha->flags; td = rd; @@ -2449,7 +2452,7 @@ acquire_grant_for_copy( shah = shared_entry_header(rgt, gref); act = active_entry_acquire(rgt, gref); - if ( rgt->gt_version == 1 ) + if ( evaluate_nospec(rgt->gt_version == 1) ) { sha2 = NULL; status = &shah->flags; @@ -3269,7 +3272,7 @@ swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b) if ( act_b->pin ) PIN_FAIL(out, GNTST_eagain, "ref b %#x busy\n", ref_b); - if ( gt->gt_version == 1 ) + if ( evaluate_nospec(gt->gt_version == 1) ) { grant_entry_v1_t shared; @@ -3818,7 +3821,7 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, rc = -EINVAL; else if ( ref >= nr_grant_entries(gt) ) rc = -ENOENT; - else if ( gt->gt_version == 1 ) + else if ( evaluate_nospec(gt->gt_version == 1) ) { const grant_entry_v1_t *sha1 = &shared_entry_v1(gt, ref); @@ -3840,7 +3843,7 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, rc = -ENXIO; else if ( !rc && status ) { - if ( gt->gt_version == 1 ) + if ( evaluate_nospec(gt->gt_version == 1) ) *status = flags; else *status = status_entry(gt, ref); @@ -3927,7 +3930,7 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) grant_write_lock(gt); - if ( gt->gt_version == 2 && (idx & XENMAPIDX_grant_table_status) ) + if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; status = true;
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 * 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 * gnttab_set_version: all accessible data is allocated for both versions * 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. * gnttab_get_shared_frame: block_speculation in gnttab_get_status_frame_mfn blocks accesses * gnttab_usage_print: cannot be triggered by the guest This is part of the speculative hardening effort. Signed-off-by: Norbert Manthey <nmanthey@amazon.de> --- Notes: v1: added additional fixes (compared to L1TF series) to: _set_status unmap_common_complete gnttab_grow_table xen/common/grant_table.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)