L1TF MDS GT v1

L1TF MDS GT v1

[Norbert Manthey]

Dear all,

This patch series attempts to mitigate the issue that have been raised in the
XSA-289 (https://xenbits.xen.org/xsa/advisory-289.html). To block speculative
execution on Intel hardware, an lfence instruction is required to make sure
that selected checks are not bypassed. Speculative out-of-bound accesses can
be prevented by using the array_index_nospec macro.

This series picks up the last remaining commit of my previous L1TF series, and
splits it into three commits to help targetting the discussion better. The
actual change is to protect three more functions for grant-table version
dependent code execution.

This is part of the speculative hardening effort. As for example mentioned
in [1], these changes also help to limit leaks via the MDS vulnerability.

Best,
Norbert

[1] https://arxiv.org/abs/1905.05726




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

[Xen-devel] L1TF MDS GT v1

[Norbert Manthey]

Dear all,

This patch series attempts to mitigate the issue that have been raised in the
XSA-289 (https://xenbits.xen.org/xsa/advisory-289.html). To block speculative
execution on Intel hardware, an lfence instruction is required to make sure
that selected checks are not bypassed. Speculative out-of-bound accesses can
be prevented by using the array_index_nospec macro.

This series picks up the last remaining commit of my previous L1TF series, and
splits it into three commits to help targetting the discussion better. The
actual change is to protect three more functions for grant-table version
dependent code execution.

This is part of the speculative hardening effort. As for example mentioned
in [1], these changes also help to limit leaks via the MDS vulnerability.

Best,
Norbert

[1] https://arxiv.org/abs/1905.05726




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

[PATCH L1TF MDS GT v1 1/3] common/grant_table: harden helpers

[Norbert Manthey]

Guests can issue grant table operations and provide guest controlled
data to them. This data is used for memory loads in helper functions
and macros. To avoid speculative out-of-bound accesses, we use the
array_index_nospec macro where applicable, or the block_speculation
macro.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

---

Notes:
  v1:  split the gnttab commit of the previous L1TF series into multiple commits

 xen/common/grant_table.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

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
@@ -37,6 +37,7 @@
 #include <xen/paging.h>
 #include <xen/keyhandler.h>
 #include <xen/vmap.h>
+#include <xen/nospec.h>
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
@@ -203,8 +204,9 @@ static inline unsigned int nr_status_frames(const struct grant_table *gt)
 }
 
 #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
-#define maptrack_entry(t, e) \
-    ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
+#define maptrack_entry(t, e)                                                   \
+    ((t)->maptrack[array_index_nospec(e, (t)->maptrack_limit) /                \
+                                    MAPTRACK_PER_PAGE][(e) % MAPTRACK_PER_PAGE])
 
 static inline unsigned int
 nr_maptrack_frames(struct grant_table *t)
@@ -226,10 +228,23 @@ nr_maptrack_frames(struct grant_table *t)
 static grant_entry_header_t *
 shared_entry_header(struct grant_table *t, grant_ref_t ref)
 {
-    if ( t->gt_version == 1 )
+    switch ( t->gt_version )
+    {
+    case 1:
+        /* Returned values should be independent of speculative execution */
+        block_speculation();
         return (grant_entry_header_t*)&shared_entry_v1(t, ref);
-    else
+
+    case 2:
+        /* Returned values should be independent of speculative execution */
+        block_speculation();
         return &shared_entry_v2(t, ref).hdr;
+    }
+
+    ASSERT_UNREACHABLE();
+    block_speculation();
+
+    return NULL;
 }
 
 /* Active grant entry - used for shadowing GTF_permit_access grants. */
@@ -634,14 +649,24 @@ static unsigned int nr_grant_entries(struct grant_table *gt)
     case 1:
         BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 1) <
                      GNTTAB_NR_RESERVED_ENTRIES);
+
+        /* Make sure we return a value independently of speculative execution */
+        block_speculation();
         return f2e(nr_grant_frames(gt), 1);
+
     case 2:
         BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 2) <
                      GNTTAB_NR_RESERVED_ENTRIES);
+
+        /* Make sure we return a value independently of speculative execution */
+        block_speculation();
         return f2e(nr_grant_frames(gt), 2);
 #undef f2e
     }
 
+    ASSERT_UNREACHABLE();
+    block_speculation();
+
     return 0;
 }
 
-- 
2.7.4




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

[Xen-devel] [PATCH L1TF MDS GT v1 1/3] common/grant_table: harden helpers

[Norbert Manthey]

Guests can issue grant table operations and provide guest controlled
data to them. This data is used for memory loads in helper functions
and macros. To avoid speculative out-of-bound accesses, we use the
array_index_nospec macro where applicable, or the block_speculation
macro.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

---

Notes:
  v1:  split the gnttab commit of the previous L1TF series into multiple commits

 xen/common/grant_table.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

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
@@ -37,6 +37,7 @@
 #include <xen/paging.h>
 #include <xen/keyhandler.h>
 #include <xen/vmap.h>
+#include <xen/nospec.h>
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
@@ -203,8 +204,9 @@ static inline unsigned int nr_status_frames(const struct grant_table *gt)
 }
 
 #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
-#define maptrack_entry(t, e) \
-    ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
+#define maptrack_entry(t, e)                                                   \
+    ((t)->maptrack[array_index_nospec(e, (t)->maptrack_limit) /                \
+                                    MAPTRACK_PER_PAGE][(e) % MAPTRACK_PER_PAGE])
 
 static inline unsigned int
 nr_maptrack_frames(struct grant_table *t)
@@ -226,10 +228,23 @@ nr_maptrack_frames(struct grant_table *t)
 static grant_entry_header_t *
 shared_entry_header(struct grant_table *t, grant_ref_t ref)
 {
-    if ( t->gt_version == 1 )
+    switch ( t->gt_version )
+    {
+    case 1:
+        /* Returned values should be independent of speculative execution */
+        block_speculation();
         return (grant_entry_header_t*)&shared_entry_v1(t, ref);
-    else
+
+    case 2:
+        /* Returned values should be independent of speculative execution */
+        block_speculation();
         return &shared_entry_v2(t, ref).hdr;
+    }
+
+    ASSERT_UNREACHABLE();
+    block_speculation();
+
+    return NULL;
 }
 
 /* Active grant entry - used for shadowing GTF_permit_access grants. */
@@ -634,14 +649,24 @@ static unsigned int nr_grant_entries(struct grant_table *gt)
     case 1:
         BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 1) <
                      GNTTAB_NR_RESERVED_ENTRIES);
+
+        /* Make sure we return a value independently of speculative execution */
+        block_speculation();
         return f2e(nr_grant_frames(gt), 1);
+
     case 2:
         BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 2) <
                      GNTTAB_NR_RESERVED_ENTRIES);
+
+        /* Make sure we return a value independently of speculative execution */
+        block_speculation();
         return f2e(nr_grant_frames(gt), 2);
 #undef f2e
     }
 
+    ASSERT_UNREACHABLE();
+    block_speculation();
+
     return 0;
 }
 
-- 
2.7.4




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

[PATCH L1TF MDS GT v1 2/3] common/grant_table: harden bound accesses

[Norbert Manthey]

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. To avoid speculative out-of-bound accesses, we
use the array_index_nospec macro where applicable, or the macro
block_speculation. Note, that the block_speculation is always used in
the calls to shared_entry_header and nr_grant_entries, so that no
additional protection is required once these functions have been
called.

Speculative execution is not blocked in case one of the following
properties is true:
 - path cannot be triggered by the guest
 - path does not return to the guest
 - path does not result in an out-of-bound access
 - path cannot be executed repeatedly
Only the combination of the above properties allows to actually leak
continuous chunks of memory. Therefore, we only add the penalty of
protective mechanisms in case a potential speculative out-of-bound
access matches all the above properties.

This commit addresses only out-of-bound accesses whose index is
directly controlled by the guest, and the index is checked before.
Potential out-of-bound accesses that are caused by speculatively
evaluating the version of the current table are not addressed in this
commit.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
---

Notes:
  v1:  adapt the comments for shared_entry_header to show that they 'also'
       block speculative execution

 xen/common/grant_table.c | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

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
@@ -988,9 +988,10 @@ map_grant_ref(
         PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
                  op->ref, rgt->domain->domain_id);
 
-    act = active_entry_acquire(rgt, op->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);
 
     /* If already pinned, check the active domid and avoid refcnt overflow. */
     if ( act->pin &&
@@ -1346,6 +1347,9 @@ unmap_common(
         goto unlock_out;
     }
 
+    /* Make sure the above bound check cannot be bypassed speculatively */
+    block_speculation();
+
     act = active_entry_acquire(rgt, op->ref);
 
     /*
@@ -1443,7 +1447,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
     struct page_info *pg;
     uint16_t *status;
 
-    if ( !op->done )
+    if ( evaluate_nospec(!op->done) )
     {
         /* unmap_common() didn't do anything - nothing to complete. */
         return;
@@ -2051,6 +2055,7 @@ gnttab_prepare_for_transfer(
         goto fail;
     }
 
+    /* This call also ensures the above check cannot be passed speculatively */
     sha = shared_entry_header(rgt, ref);
 
     scombo.word = *(u32 *)&sha->flags;
@@ -2248,7 +2253,12 @@ gnttab_transfer(
         spin_unlock(&e->page_alloc_lock);
         okay = gnttab_prepare_for_transfer(e, d, gop.ref);
 
-        if ( unlikely(!okay || assign_pages(e, page, 0, MEMF_no_refcount)) )
+        /*
+         * Make sure the reference bound check in gnttab_prepare_for_transfer
+         * is respected and speculative execution is blocked accordingly
+         */
+        if ( unlikely(!evaluate_nospec(okay)) ||
+            unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) )
         {
             bool drop_dom_ref;
 
@@ -2435,8 +2445,10 @@ 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 also ensures the above check cannot be passed speculatively */
     shah = shared_entry_header(rgt, gref);
+    act = active_entry_acquire(rgt, gref);
+
     if ( rgt->gt_version == 1 )
     {
         sha2 = NULL;
@@ -2853,6 +2865,9 @@ static int gnttab_copy_buf(const struct gnttab_copy *op,
                  op->dest.offset, dest->ptr.offset,
                  op->len, dest->len);
 
+    /* Make sure the above checks are not bypassed speculatively */
+    block_speculation();
+
     memcpy(dest->virt + op->dest.offset, src->virt + op->source.offset,
            op->len);
     gnttab_mark_dirty(dest->domain, dest->mfn);
@@ -2972,7 +2987,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
     struct grant_table *gt = currd->grant_table;
     grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
     int res;
-    unsigned int i;
+    unsigned int i, nr_ents;
 
     if ( copy_from_guest(&op, uop, 1) )
         return -EFAULT;
@@ -2996,7 +3011,8 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
      * are allowed to be in use (xenstore/xenconsole keeps them mapped).
      * (You need to change the version number for e.g. kexec.)
      */
-    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
+    nr_ents = nr_grant_entries(gt);
+    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_ents; i++ )
     {
         if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
         {
@@ -3238,6 +3254,9 @@ swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
     if ( unlikely(ref_b >= nr_grant_entries(d->grant_table)))
         PIN_FAIL(out, GNTST_bad_gntref, "Bad ref-b %#x\n", ref_b);
 
+    /* Make sure the above checks are not bypassed speculatively */
+    block_speculation();
+
     /* Swapping the same ref is a no-op. */
     if ( ref_a == ref_b )
         goto out;
@@ -3707,13 +3726,14 @@ void grant_table_warn_active_grants(struct domain *d)
     struct grant_table *gt = d->grant_table;
     struct active_grant_entry *act;
     grant_ref_t ref;
-    unsigned int nr_active = 0;
+    unsigned int nr_active = 0, nr_ents;
 
 #define WARN_GRANT_MAX 10
 
     grant_read_lock(gt);
 
-    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
+    nr_ents = nr_grant_entries(gt);
+    for ( ref = 0; ref != nr_ents; ref++ )
     {
         act = active_entry_acquire(gt, ref);
         if ( !act->pin )
@@ -3863,6 +3883,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]));
     return 0;
 }
@@ -3962,6 +3985,7 @@ static void gnttab_usage_print(struct domain *rd)
     int first = 1;
     grant_ref_t ref;
     struct grant_table *gt = rd->grant_table;
+    unsigned int nr_ents;
 
     printk("      -------- active --------       -------- shared --------\n");
     printk("[ref] localdom mfn      pin          localdom gmfn     flags\n");
@@ -3974,7 +3998,8 @@ static void gnttab_usage_print(struct domain *rd)
            nr_grant_frames(gt), gt->max_grant_frames,
            nr_maptrack_frames(gt), gt->max_maptrack_frames);
 
-    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
+    nr_ents = nr_grant_entries(gt);
+    for ( ref = 0; ref != nr_ents; ref++ )
     {
         struct active_grant_entry *act;
         struct grant_entry_header *sha;
-- 
2.7.4




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

[Xen-devel] [PATCH L1TF MDS GT v1 2/3] common/grant_table: harden bound accesses

[Norbert Manthey]

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. To avoid speculative out-of-bound accesses, we
use the array_index_nospec macro where applicable, or the macro
block_speculation. Note, that the block_speculation is always used in
the calls to shared_entry_header and nr_grant_entries, so that no
additional protection is required once these functions have been
called.

Speculative execution is not blocked in case one of the following
properties is true:
 - path cannot be triggered by the guest
 - path does not return to the guest
 - path does not result in an out-of-bound access
 - path cannot be executed repeatedly
Only the combination of the above properties allows to actually leak
continuous chunks of memory. Therefore, we only add the penalty of
protective mechanisms in case a potential speculative out-of-bound
access matches all the above properties.

This commit addresses only out-of-bound accesses whose index is
directly controlled by the guest, and the index is checked before.
Potential out-of-bound accesses that are caused by speculatively
evaluating the version of the current table are not addressed in this
commit.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
---

Notes:
  v1:  adapt the comments for shared_entry_header to show that they 'also'
       block speculative execution

 xen/common/grant_table.c | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

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
@@ -988,9 +988,10 @@ map_grant_ref(
         PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
                  op->ref, rgt->domain->domain_id);
 
-    act = active_entry_acquire(rgt, op->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);
 
     /* If already pinned, check the active domid and avoid refcnt overflow. */
     if ( act->pin &&
@@ -1346,6 +1347,9 @@ unmap_common(
         goto unlock_out;
     }
 
+    /* Make sure the above bound check cannot be bypassed speculatively */
+    block_speculation();
+
     act = active_entry_acquire(rgt, op->ref);
 
     /*
@@ -1443,7 +1447,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
     struct page_info *pg;
     uint16_t *status;
 
-    if ( !op->done )
+    if ( evaluate_nospec(!op->done) )
     {
         /* unmap_common() didn't do anything - nothing to complete. */
         return;
@@ -2051,6 +2055,7 @@ gnttab_prepare_for_transfer(
         goto fail;
     }
 
+    /* This call also ensures the above check cannot be passed speculatively */
     sha = shared_entry_header(rgt, ref);
 
     scombo.word = *(u32 *)&sha->flags;
@@ -2248,7 +2253,12 @@ gnttab_transfer(
         spin_unlock(&e->page_alloc_lock);
         okay = gnttab_prepare_for_transfer(e, d, gop.ref);
 
-        if ( unlikely(!okay || assign_pages(e, page, 0, MEMF_no_refcount)) )
+        /*
+         * Make sure the reference bound check in gnttab_prepare_for_transfer
+         * is respected and speculative execution is blocked accordingly
+         */
+        if ( unlikely(!evaluate_nospec(okay)) ||
+            unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) )
         {
             bool drop_dom_ref;
 
@@ -2435,8 +2445,10 @@ 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 also ensures the above check cannot be passed speculatively */
     shah = shared_entry_header(rgt, gref);
+    act = active_entry_acquire(rgt, gref);
+
     if ( rgt->gt_version == 1 )
     {
         sha2 = NULL;
@@ -2853,6 +2865,9 @@ static int gnttab_copy_buf(const struct gnttab_copy *op,
                  op->dest.offset, dest->ptr.offset,
                  op->len, dest->len);
 
+    /* Make sure the above checks are not bypassed speculatively */
+    block_speculation();
+
     memcpy(dest->virt + op->dest.offset, src->virt + op->source.offset,
            op->len);
     gnttab_mark_dirty(dest->domain, dest->mfn);
@@ -2972,7 +2987,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
     struct grant_table *gt = currd->grant_table;
     grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
     int res;
-    unsigned int i;
+    unsigned int i, nr_ents;
 
     if ( copy_from_guest(&op, uop, 1) )
         return -EFAULT;
@@ -2996,7 +3011,8 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
      * are allowed to be in use (xenstore/xenconsole keeps them mapped).
      * (You need to change the version number for e.g. kexec.)
      */
-    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
+    nr_ents = nr_grant_entries(gt);
+    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_ents; i++ )
     {
         if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
         {
@@ -3238,6 +3254,9 @@ swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
     if ( unlikely(ref_b >= nr_grant_entries(d->grant_table)))
         PIN_FAIL(out, GNTST_bad_gntref, "Bad ref-b %#x\n", ref_b);
 
+    /* Make sure the above checks are not bypassed speculatively */
+    block_speculation();
+
     /* Swapping the same ref is a no-op. */
     if ( ref_a == ref_b )
         goto out;
@@ -3707,13 +3726,14 @@ void grant_table_warn_active_grants(struct domain *d)
     struct grant_table *gt = d->grant_table;
     struct active_grant_entry *act;
     grant_ref_t ref;
-    unsigned int nr_active = 0;
+    unsigned int nr_active = 0, nr_ents;
 
 #define WARN_GRANT_MAX 10
 
     grant_read_lock(gt);
 
-    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
+    nr_ents = nr_grant_entries(gt);
+    for ( ref = 0; ref != nr_ents; ref++ )
     {
         act = active_entry_acquire(gt, ref);
         if ( !act->pin )
@@ -3863,6 +3883,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]));
     return 0;
 }
@@ -3962,6 +3985,7 @@ static void gnttab_usage_print(struct domain *rd)
     int first = 1;
     grant_ref_t ref;
     struct grant_table *gt = rd->grant_table;
+    unsigned int nr_ents;
 
     printk("      -------- active --------       -------- shared --------\n");
     printk("[ref] localdom mfn      pin          localdom gmfn     flags\n");
@@ -3974,7 +3998,8 @@ static void gnttab_usage_print(struct domain *rd)
            nr_grant_frames(gt), gt->max_grant_frames,
            nr_maptrack_frames(gt), gt->max_maptrack_frames);
 
-    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
+    nr_ents = nr_grant_entries(gt);
+    for ( ref = 0; ref != nr_ents; ref++ )
     {
         struct active_grant_entry *act;
         struct grant_entry_header *sha;
-- 
2.7.4




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

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

[Norbert Manthey]

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(-)

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;
-- 
2.7.4




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

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

[Norbert Manthey]

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(-)

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;
-- 
2.7.4




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

Re: [PATCH L1TF MDS GT v1 1/3] common/grant_table: harden helpers

[Jan Beulich]

>>> 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 for memory loads in helper functions
> and macros. To avoid speculative out-of-bound accesses, we use the
> array_index_nospec macro where applicable, or the block_speculation
> macro.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [Xen-devel] [PATCH L1TF MDS GT v1 1/3] common/grant_table: harden helpers

[Jan Beulich]

>>> 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 for memory loads in helper functions
> and macros. To avoid speculative out-of-bound accesses, we use the
> array_index_nospec macro where applicable, or the block_speculation
> macro.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH L1TF MDS GT v1 2/3] common/grant_table: harden bound accesses

[Jan Beulich]

>>> 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. To avoid speculative out-of-bound accesses, we
> use the array_index_nospec macro where applicable, or the macro
> block_speculation. Note, that the block_speculation is always used in

s/always/already/ ?

> the calls to shared_entry_header and nr_grant_entries, so that no
> additional protection is required once these functions have been
> called.

Isn't this too broad a statement? There's some protection, but not
for just anything that follows.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -988,9 +988,10 @@ map_grant_ref(
>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>                   op->ref, rgt->domain->domain_id);
>  
> -    act = active_entry_acquire(rgt, op->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);

I know we've been there before, but what guarantees that the
compiler won't reload op->ref from memory for either of the
latter two accesses? In fact afaict it always will, due to the
memory clobber in alternative().

> @@ -3863,6 +3883,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]));
>      return 0;
>  }

Why don't you use array_index_nospec() here? And how come
speculation into gnttab_grow_table() is fine a few lines above?
And what about the similar code in gnttab_get_shared_frame_mfn()?

Jan



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

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

[Jan Beulich]

>>> 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. To avoid speculative out-of-bound accesses, we
> use the array_index_nospec macro where applicable, or the macro
> block_speculation. Note, that the block_speculation is always used in

s/always/already/ ?

> the calls to shared_entry_header and nr_grant_entries, so that no
> additional protection is required once these functions have been
> called.

Isn't this too broad a statement? There's some protection, but not
for just anything that follows.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -988,9 +988,10 @@ map_grant_ref(
>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>                   op->ref, rgt->domain->domain_id);
>  
> -    act = active_entry_acquire(rgt, op->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);

I know we've been there before, but what guarantees that the
compiler won't reload op->ref from memory for either of the
latter two accesses? In fact afaict it always will, due to the
memory clobber in alternative().

> @@ -3863,6 +3883,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]));
>      return 0;
>  }

Why don't you use array_index_nospec() here? And how come
speculation into gnttab_grow_table() is fine a few lines above?
And what about the similar code in gnttab_get_shared_frame_mfn()?

Jan



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

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

[Jan Beulich]

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



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

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

[Jan Beulich]

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



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

Re: [PATCH L1TF MDS GT v1 2/3] common/grant_table: harden bound accesses

[Norbert Manthey]

On 5/23/19 16:17, 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. To avoid speculative out-of-bound accesses, we
>> use the array_index_nospec macro where applicable, or the macro
>> block_speculation. Note, that the block_speculation is always used in
> s/always/already/ ?
They both work, but the 'always' underlines that a caller can rely on
the fact that this will happen on all execution path of that function.
Hence, I like to stick to 'always' here.
>
>> the calls to shared_entry_header and nr_grant_entries, so that no
>> additional protection is required once these functions have been
>> called.
> Isn't this too broad a statement? There's some protection, but not
> for just anything that follows.
You are right, to given protection is that any bound check that could
have been bypassed speculatively is enforced after calling one of the
two functions. I will rephrase the commit message accordingly.
>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -988,9 +988,10 @@ map_grant_ref(
>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>                   op->ref, rgt->domain->domain_id);
>>  
>> -    act = active_entry_acquire(rgt, op->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);
> I know we've been there before, but what guarantees that the
> compiler won't reload op->ref from memory for either of the
> latter two accesses? In fact afaict it always will, due to the
> memory clobber in alternative().
The compiler can reload op->ref from memory, that is fine here, as the
bound check happens above, and the shared_entry call comes with an
lfence() by now, so that we will not continue executing speculatively
with op->ref being out-of-bounds, independently of whether it's from
memory or registers.
>
>> @@ -3863,6 +3883,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]));
>>      return 0;
>>  }
> Why don't you use array_index_nospec() here? And how come
There is no specific reason. I will swap.
> speculation into gnttab_grow_table() is fine a few lines above?
I do not see a reason why it would be bad to enter that function
speculatively. There are no accesses that would have to be protected by
extra checks, afaict. Otherwise, that function should be protected by
its own.
> And what about the similar code in gnttab_get_shared_frame_mfn()?
I will add an array_nospec_index there as well.
>
> Jan
>
>




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

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

[Norbert Manthey]

On 5/23/19 16:17, 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. To avoid speculative out-of-bound accesses, we
>> use the array_index_nospec macro where applicable, or the macro
>> block_speculation. Note, that the block_speculation is always used in
> s/always/already/ ?
They both work, but the 'always' underlines that a caller can rely on
the fact that this will happen on all execution path of that function.
Hence, I like to stick to 'always' here.
>
>> the calls to shared_entry_header and nr_grant_entries, so that no
>> additional protection is required once these functions have been
>> called.
> Isn't this too broad a statement? There's some protection, but not
> for just anything that follows.
You are right, to given protection is that any bound check that could
have been bypassed speculatively is enforced after calling one of the
two functions. I will rephrase the commit message accordingly.
>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -988,9 +988,10 @@ map_grant_ref(
>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>                   op->ref, rgt->domain->domain_id);
>>  
>> -    act = active_entry_acquire(rgt, op->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);
> I know we've been there before, but what guarantees that the
> compiler won't reload op->ref from memory for either of the
> latter two accesses? In fact afaict it always will, due to the
> memory clobber in alternative().
The compiler can reload op->ref from memory, that is fine here, as the
bound check happens above, and the shared_entry call comes with an
lfence() by now, so that we will not continue executing speculatively
with op->ref being out-of-bounds, independently of whether it's from
memory or registers.
>
>> @@ -3863,6 +3883,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]));
>>      return 0;
>>  }
> Why don't you use array_index_nospec() here? And how come
There is no specific reason. I will swap.
> speculation into gnttab_grow_table() is fine a few lines above?
I do not see a reason why it would be bad to enter that function
speculatively. There are no accesses that would have to be protected by
extra checks, afaict. Otherwise, that function should be protected by
its own.
> And what about the similar code in gnttab_get_shared_frame_mfn()?
I will add an array_nospec_index there as well.
>
> Jan
>
>




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

Re: [PATCH L1TF MDS GT v1 2/3] common/grant_table: harden bound accesses

[Jan Beulich]

>>> On 24.05.19 at 11:54, <nmanthey@amazon.de> wrote:
> On 5/23/19 16:17, 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. To avoid speculative out-of-bound accesses, we
>>> use the array_index_nospec macro where applicable, or the macro
>>> block_speculation. Note, that the block_speculation is always used in
>> s/always/already/ ?
> They both work, but the 'always' underlines that a caller can rely on
> the fact that this will happen on all execution path of that function.
> Hence, I like to stick to 'always' here.

Well, I'm not a native speaker, but to me "always" doesn't express
what you want to express here. What you mean I'd word "... is used
on all paths of ..."

>>> the calls to shared_entry_header and nr_grant_entries, so that no
>>> additional protection is required once these functions have been
>>> called.

As an aside, your use of "in the calls to" looks also misleading to me,
because the fences sit in the functions, not at the call sites.

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -988,9 +988,10 @@ map_grant_ref(
>>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>                   op->ref, rgt->domain->domain_id);
>>>  
>>> -    act = active_entry_acquire(rgt, op->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);
>> I know we've been there before, but what guarantees that the
>> compiler won't reload op->ref from memory for either of the
>> latter two accesses? In fact afaict it always will, due to the
>> memory clobber in alternative().
> The compiler can reload op->ref from memory, that is fine here, as the
> bound check happens above, and the shared_entry call comes with an
> lfence() by now, so that we will not continue executing speculatively
> with op->ref being out-of-bounds, independently of whether it's from
> memory or registers.

I don't buy this argumentation: In particular if the cache line got
flushed for whatever reason, the load may take almost arbitrarily
long, opening up a large speculation window again using the
destination register of the load (whatever - not bounds checked -
value that ends up holding).

>>> @@ -3863,6 +3883,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]));
>>>      return 0;
>>>  }
>> Why don't you use array_index_nospec() here? And how come
> There is no specific reason. I will swap.
>> speculation into gnttab_grow_table() is fine a few lines above?
> I do not see a reason why it would be bad to enter that function
> speculatively. There are no accesses that would have to be protected by
> extra checks, afaict. Otherwise, that function should be protected by
> its own.

Which in fact happens, but only in patch 3. This may be worth saying
explicitly here.

Jan



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

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

[Jan Beulich]

>>> On 24.05.19 at 11:54, <nmanthey@amazon.de> wrote:
> On 5/23/19 16:17, 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. To avoid speculative out-of-bound accesses, we
>>> use the array_index_nospec macro where applicable, or the macro
>>> block_speculation. Note, that the block_speculation is always used in
>> s/always/already/ ?
> They both work, but the 'always' underlines that a caller can rely on
> the fact that this will happen on all execution path of that function.
> Hence, I like to stick to 'always' here.

Well, I'm not a native speaker, but to me "always" doesn't express
what you want to express here. What you mean I'd word "... is used
on all paths of ..."

>>> the calls to shared_entry_header and nr_grant_entries, so that no
>>> additional protection is required once these functions have been
>>> called.

As an aside, your use of "in the calls to" looks also misleading to me,
because the fences sit in the functions, not at the call sites.

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -988,9 +988,10 @@ map_grant_ref(
>>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>                   op->ref, rgt->domain->domain_id);
>>>  
>>> -    act = active_entry_acquire(rgt, op->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);
>> I know we've been there before, but what guarantees that the
>> compiler won't reload op->ref from memory for either of the
>> latter two accesses? In fact afaict it always will, due to the
>> memory clobber in alternative().
> The compiler can reload op->ref from memory, that is fine here, as the
> bound check happens above, and the shared_entry call comes with an
> lfence() by now, so that we will not continue executing speculatively
> with op->ref being out-of-bounds, independently of whether it's from
> memory or registers.

I don't buy this argumentation: In particular if the cache line got
flushed for whatever reason, the load may take almost arbitrarily
long, opening up a large speculation window again using the
destination register of the load (whatever - not bounds checked -
value that ends up holding).

>>> @@ -3863,6 +3883,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]));
>>>      return 0;
>>>  }
>> Why don't you use array_index_nospec() here? And how come
> There is no specific reason. I will swap.
>> speculation into gnttab_grow_table() is fine a few lines above?
> I do not see a reason why it would be bad to enter that function
> speculatively. There are no accesses that would have to be protected by
> extra checks, afaict. Otherwise, that function should be protected by
> its own.

Which in fact happens, but only in patch 3. This may be worth saying
explicitly here.

Jan



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

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

[Norbert Manthey]

Sorry for the late reply. I try to pick up where we left the discussion
the last time.

On 5/24/19 13:10, Jan Beulich wrote:
>>>> On 24.05.19 at 11:54, <nmanthey@amazon.de> wrote:
>> On 5/23/19 16:17, 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. To avoid speculative out-of-bound accesses, we
>>>> use the array_index_nospec macro where applicable, or the macro
>>>> block_speculation. Note, that the block_speculation is always used in
>>> s/always/already/ ?
>> They both work, but the 'always' underlines that a caller can rely on
>> the fact that this will happen on all execution path of that function.
>> Hence, I like to stick to 'always' here.
> Well, I'm not a native speaker, but to me "always" doesn't express
> what you want to express here. What you mean I'd word "... is used
> on all paths of ..."
I will rephrase accordingly.
>
>>>> the calls to shared_entry_header and nr_grant_entries, so that no
>>>> additional protection is required once these functions have been
>>>> called.
> As an aside, your use of "in the calls to" looks also misleading to me,
> because the fences sit in the functions, not at the call sites.
Will fix.
>
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -988,9 +988,10 @@ map_grant_ref(
>>>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>>                   op->ref, rgt->domain->domain_id);
>>>>  
>>>> -    act = active_entry_acquire(rgt, op->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);
>>> I know we've been there before, but what guarantees that the
>>> compiler won't reload op->ref from memory for either of the
>>> latter two accesses? In fact afaict it always will, due to the
>>> memory clobber in alternative().
>> The compiler can reload op->ref from memory, that is fine here, as the
>> bound check happens above, and the shared_entry call comes with an
>> lfence() by now, so that we will not continue executing speculatively
>> with op->ref being out-of-bounds, independently of whether it's from
>> memory or registers.
> I don't buy this argumentation: In particular if the cache line got
> flushed for whatever reason, the load may take almost arbitrarily
> long, opening up a large speculation window again using the
> destination register of the load (whatever - not bounds checked -
> value that ends up holding).
I agree, the given protection does not force the CPU to pick up the
fixed value. As you already noticed, the presented fix might not work in
all cases, but is among the suitable solutions we have today to prevent
simple user controlled out-of-bound accesses during speculation. Relying
on the stale value of the register that might be used during speculation
makes a potential out-of-bound access much more difficult. Hence, the
proposed solution looks good enough to me.
>
>>>> @@ -3863,6 +3883,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]));
>>>>      return 0;
>>>>  }
>>> Why don't you use array_index_nospec() here? And how come
>> There is no specific reason. I will swap.
>>> speculation into q() is fine a few lines above?
>> I do not see a reason why it would be bad to enter that function
>> speculatively. There are no accesses that would have to be protected by
>> extra checks, afaict. Otherwise, that function should be protected by
>> its own.
> Which in fact happens, but only in patch 3. This may be worth saying
> explicitly here.

Do you want me to explicitly mention this in the commit message, or add
a comment here, which I have to drop in patch 3 again? For now, I'd just
leave it as is, as the version based fixes are handled in the other commit.

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


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

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

[Norbert Manthey]

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


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

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

[Jan Beulich]

On 08.07.2019 14:58, Norbert Manthey wrote:
> On 5/24/19 13:10, Jan Beulich wrote:
>>>>> On 24.05.19 at 11:54, <nmanthey@amazon.de> wrote:
>>> On 5/23/19 16:17, Jan Beulich wrote:
>>>>>>> On 21.05.19 at 09:45, <nmanthey@amazon.de> wrote:
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -988,9 +988,10 @@ map_grant_ref(
>>>>>           PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>>>                    op->ref, rgt->domain->domain_id);
>>>>>   
>>>>> -    act = active_entry_acquire(rgt, op->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);
>>>> I know we've been there before, but what guarantees that the
>>>> compiler won't reload op->ref from memory for either of the
>>>> latter two accesses? In fact afaict it always will, due to the
>>>> memory clobber in alternative().
>>> The compiler can reload op->ref from memory, that is fine here, as the
>>> bound check happens above, and the shared_entry call comes with an
>>> lfence() by now, so that we will not continue executing speculatively
>>> with op->ref being out-of-bounds, independently of whether it's from
>>> memory or registers.
>> I don't buy this argumentation: In particular if the cache line got
>> flushed for whatever reason, the load may take almost arbitrarily
>> long, opening up a large speculation window again using the
>> destination register of the load (whatever - not bounds checked -
>> value that ends up holding).
> I agree, the given protection does not force the CPU to pick up the
> fixed value. As you already noticed, the presented fix might not work in
> all cases, but is among the suitable solutions we have today to prevent
> simple user controlled out-of-bound accesses during speculation. Relying
> on the stale value of the register that might be used during speculation
> makes a potential out-of-bound access much more difficult. Hence, the
> proposed solution looks good enough to me.

But using a local variable further reduces the risk afaict: Either
the compiler puts it into a register, in which case we're entirely
fine. Or it puts it on the stack, which I assume is more likely to
stay in cache than a reference to some other data structure (iirc
also on the stack, but not in the current frame).

>>>>> @@ -3863,6 +3883,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]));
>>>>>       return 0;
>>>>>   }
>>>> Why don't you use array_index_nospec() here? And how come
>>> There is no specific reason. I will swap.
>>>> speculation into q() is fine a few lines above?
>>> I do not see a reason why it would be bad to enter that function
>>> speculatively. There are no accesses that would have to be protected by
>>> extra checks, afaict. Otherwise, that function should be protected by
>>> its own.
>> Which in fact happens, but only in patch 3. This may be worth saying
>> explicitly here.
> 
> Do you want me to explicitly mention this in the commit message, or add
> a comment here, which I have to drop in patch 3 again? For now, I'd just
> leave it as is, as the version based fixes are handled in the other commit.

A commit message remark would both help understand things now and
in the future, and at the same time avoid me or someone else re-
raising the question next time round, not the least because of the
noticable gaps between versions.

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

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

[Jan Beulich]

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Norbert Manthey]

On 7/10/19 05:04, Jan Beulich wrote:
> On 08.07.2019 14:58, Norbert Manthey wrote:
>> On 5/24/19 13:10, Jan Beulich wrote:
>>>>>> On 24.05.19 at 11:54, <nmanthey@amazon.de> wrote:
>>>> On 5/23/19 16:17, Jan Beulich wrote:
>>>>>>>> On 21.05.19 at 09:45, <nmanthey@amazon.de> wrote:
>>>>>> --- a/xen/common/grant_table.c
>>>>>> +++ b/xen/common/grant_table.c
>>>>>> @@ -988,9 +988,10 @@ map_grant_ref(
>>>>>>           PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>>>>                    op->ref, rgt->domain->domain_id);
>>>>>>   
>>>>>> -    act = active_entry_acquire(rgt, op->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);
>>>>> I know we've been there before, but what guarantees that the
>>>>> compiler won't reload op->ref from memory for either of the
>>>>> latter two accesses? In fact afaict it always will, due to the
>>>>> memory clobber in alternative().
>>>> The compiler can reload op->ref from memory, that is fine here, as the
>>>> bound check happens above, and the shared_entry call comes with an
>>>> lfence() by now, so that we will not continue executing speculatively
>>>> with op->ref being out-of-bounds, independently of whether it's from
>>>> memory or registers.
>>> I don't buy this argumentation: In particular if the cache line got
>>> flushed for whatever reason, the load may take almost arbitrarily
>>> long, opening up a large speculation window again using the
>>> destination register of the load (whatever - not bounds checked -
>>> value that ends up holding).
>> I agree, the given protection does not force the CPU to pick up the
>> fixed value. As you already noticed, the presented fix might not work in
>> all cases, but is among the suitable solutions we have today to prevent
>> simple user controlled out-of-bound accesses during speculation. Relying
>> on the stale value of the register that might be used during speculation
>> makes a potential out-of-bound access much more difficult. Hence, the
>> proposed solution looks good enough to me.
> But using a local variable further reduces the risk afaict: Either
> the compiler puts it into a register, in which case we're entirely
> fine. Or it puts it on the stack, which I assume is more likely to
> stay in cache than a reference to some other data structure (iirc
> also on the stack, but not in the current frame).
If you want me to introduce a local variable, I can do that. I remember
we had discussions around that as well.
>
>>>>>> @@ -3863,6 +3883,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]));
>>>>>>       return 0;
>>>>>>   }
>>>>> Why don't you use array_index_nospec() here? And how come
>>>> There is no specific reason. I will swap.
>>>>> speculation into q() is fine a few lines above?
>>>> I do not see a reason why it would be bad to enter that function
>>>> speculatively. There are no accesses that would have to be protected by
>>>> extra checks, afaict. Otherwise, that function should be protected by
>>>> its own.
>>> Which in fact happens, but only in patch 3. This may be worth saying
>>> explicitly here.
>> Do you want me to explicitly mention this in the commit message, or add
>> a comment here, which I have to drop in patch 3 again? For now, I'd just
>> leave it as is, as the version based fixes are handled in the other commit.
> A commit message remark would both help understand things now and
> in the future, and at the same time avoid me or someone else re-
> raising the question next time round, not the least because of the
> noticable gaps between versions.

I will extend the commit message accordingly.

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


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

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

[Norbert Manthey]

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


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

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

[Jan Beulich]

On 10.07.2019 10:54, Norbert Manthey wrote:
> On 7/10/19 05:04, Jan Beulich wrote:
>> On 08.07.2019 14:58, Norbert Manthey wrote:
>>> On 5/24/19 13:10, Jan Beulich wrote:
>>>>>>> On 24.05.19 at 11:54, <nmanthey@amazon.de> wrote:
>>>>> On 5/23/19 16:17, Jan Beulich wrote:
>>>>>>>>> On 21.05.19 at 09:45, <nmanthey@amazon.de> wrote:
>>>>>>> --- a/xen/common/grant_table.c
>>>>>>> +++ b/xen/common/grant_table.c
>>>>>>> @@ -988,9 +988,10 @@ map_grant_ref(
>>>>>>>            PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>>>>>>                     op->ref, rgt->domain->domain_id);
>>>>>>>    
>>>>>>> -    act = active_entry_acquire(rgt, op->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);
>>>>>> I know we've been there before, but what guarantees that the
>>>>>> compiler won't reload op->ref from memory for either of the
>>>>>> latter two accesses? In fact afaict it always will, due to the
>>>>>> memory clobber in alternative().
>>>>> The compiler can reload op->ref from memory, that is fine here, as the
>>>>> bound check happens above, and the shared_entry call comes with an
>>>>> lfence() by now, so that we will not continue executing speculatively
>>>>> with op->ref being out-of-bounds, independently of whether it's from
>>>>> memory or registers.
>>>> I don't buy this argumentation: In particular if the cache line got
>>>> flushed for whatever reason, the load may take almost arbitrarily
>>>> long, opening up a large speculation window again using the
>>>> destination register of the load (whatever - not bounds checked -
>>>> value that ends up holding).
>>> I agree, the given protection does not force the CPU to pick up the
>>> fixed value. As you already noticed, the presented fix might not work in
>>> all cases, but is among the suitable solutions we have today to prevent
>>> simple user controlled out-of-bound accesses during speculation. Relying
>>> on the stale value of the register that might be used during speculation
>>> makes a potential out-of-bound access much more difficult. Hence, the
>>> proposed solution looks good enough to me.
>> But using a local variable further reduces the risk afaict: Either
>> the compiler puts it into a register, in which case we're entirely
>> fine. Or it puts it on the stack, which I assume is more likely to
>> stay in cache than a reference to some other data structure (iirc
>> also on the stack, but not in the current frame).
> If you want me to introduce a local variable, I can do that. I remember
> we had discussions around that as well.

I think this would be (at least slightly) better, yes.

Jan

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