L1TF Patch Series v8 (was SpectreV1+L1TF)

L1TF Patch Series v8 (was SpectreV1+L1TF)

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

The major change compared to version 7 is in grant_table handling, making use
of the block_speculation call that is introduced in frequently used functions.

Best,
Norbert




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 v8 1/9] xen/evtchn: block speculative out-of-bound accesses

[Norbert Manthey]

Guests can issue event channel interaction with guest specified data.
To avoid speculative out-of-bound accesses, we use the nospec macros,
or the domain_vcpu function. Where appropriate, we use the vcpu_id of
the seleceted vcpu instead of the parameter that can be influenced by
the guest, so that only one access needs to be protected.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

---

Notes:
  v8: add reviewed-by
      drop blank line change

 xen/common/event_channel.c | 28 +++++++++++++++++-----------
 xen/common/event_fifo.c    | 13 ++++++++++---
 xen/include/xen/event.h    |  5 +++--
 3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -365,11 +365,16 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
     if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
         return -EINVAL;
 
+   /*
+    * Make sure the guest controlled value virq is bounded even during
+    * speculative execution.
+    */
+    virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
+
     if ( virq_is_global(virq) && (vcpu != 0) )
         return -EINVAL;
 
-    if ( (vcpu < 0) || (vcpu >= d->max_vcpus) ||
-         ((v = d->vcpu[vcpu]) == NULL) )
+    if ( (v = domain_vcpu(d, vcpu)) == NULL )
         return -ENOENT;
 
     spin_lock(&d->event_lock);
@@ -418,8 +423,7 @@ static long evtchn_bind_ipi(evtchn_bind_ipi_t *bind)
     int            port, vcpu = bind->vcpu;
     long           rc = 0;
 
-    if ( (vcpu < 0) || (vcpu >= d->max_vcpus) ||
-         (d->vcpu[vcpu] == NULL) )
+    if ( domain_vcpu(d, vcpu) == NULL )
         return -ENOENT;
 
     spin_lock(&d->event_lock);
@@ -930,8 +934,10 @@ long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id)
     struct domain *d = current->domain;
     struct evtchn *chn;
     long           rc = 0;
+    struct vcpu   *v;
 
-    if ( (vcpu_id >= d->max_vcpus) || (d->vcpu[vcpu_id] == NULL) )
+    /* Use the vcpu info to prevent speculative out-of-bound accesses */
+    if ( (v = domain_vcpu(d, vcpu_id)) == NULL )
         return -ENOENT;
 
     spin_lock(&d->event_lock);
@@ -955,22 +961,22 @@ long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id)
     {
     case ECS_VIRQ:
         if ( virq_is_global(chn->u.virq) )
-            chn->notify_vcpu_id = vcpu_id;
+            chn->notify_vcpu_id = v->vcpu_id;
         else
             rc = -EINVAL;
         break;
     case ECS_UNBOUND:
     case ECS_INTERDOMAIN:
-        chn->notify_vcpu_id = vcpu_id;
+        chn->notify_vcpu_id = v->vcpu_id;
         break;
     case ECS_PIRQ:
-        if ( chn->notify_vcpu_id == vcpu_id )
+        if ( chn->notify_vcpu_id == v->vcpu_id )
             break;
         unlink_pirq_port(chn, d->vcpu[chn->notify_vcpu_id]);
-        chn->notify_vcpu_id = vcpu_id;
+        chn->notify_vcpu_id = v->vcpu_id;
         pirq_set_affinity(d, chn->u.pirq.irq,
-                          cpumask_of(d->vcpu[vcpu_id]->processor));
-        link_pirq_port(port, chn, d->vcpu[vcpu_id]);
+                          cpumask_of(v->processor));
+        link_pirq_port(port, chn, v);
         break;
     default:
         rc = -EINVAL;
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -33,7 +33,8 @@ static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d,
      */
     smp_rmb();
 
-    p = port / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
+    p = array_index_nospec(port / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE,
+                           d->evtchn_fifo->num_evtchns);
     w = port % EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
 
     return d->evtchn_fifo->event_array[p] + w;
@@ -516,14 +517,20 @@ int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
     gfn     = init_control->control_gfn;
     offset  = init_control->offset;
 
-    if ( vcpu_id >= d->max_vcpus || !d->vcpu[vcpu_id] )
+    if ( (v = domain_vcpu(d, vcpu_id)) == NULL )
         return -ENOENT;
-    v = d->vcpu[vcpu_id];
 
     /* Must not cross page boundary. */
     if ( offset > (PAGE_SIZE - sizeof(evtchn_fifo_control_block_t)) )
         return -EINVAL;
 
+    /*
+     * Make sure the guest controlled value offset is bounded even during
+     * speculative execution.
+     */
+    offset = array_index_nospec(offset,
+                           PAGE_SIZE - sizeof(evtchn_fifo_control_block_t) + 1);
+
     /* Must be 8-bytes aligned. */
     if ( offset & (8 - 1) )
         return -EINVAL;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -13,6 +13,7 @@
 #include <xen/smp.h>
 #include <xen/softirq.h>
 #include <xen/bitops.h>
+#include <xen/nospec.h>
 #include <asm/event.h>
 
 /*
@@ -103,7 +104,7 @@ void arch_evtchn_inject(struct vcpu *v);
  * The first bucket is directly accessed via d->evtchn.
  */
 #define group_from_port(d, p) \
-    ((d)->evtchn_group[(p) / EVTCHNS_PER_GROUP])
+    array_access_nospec((d)->evtchn_group, (p) / EVTCHNS_PER_GROUP)
 #define bucket_from_port(d, p) \
     ((group_from_port(d, p))[((p) % EVTCHNS_PER_GROUP) / EVTCHNS_PER_BUCKET])
 
@@ -117,7 +118,7 @@ static inline bool_t port_is_valid(struct domain *d, unsigned int p)
 static inline struct evtchn *evtchn_from_port(struct domain *d, unsigned int p)
 {
     if ( p < EVTCHNS_PER_BUCKET )
-        return &d->evtchn[p];
+        return &d->evtchn[array_index_nospec(p, EVTCHNS_PER_BUCKET)];
     return bucket_from_port(d, p) + (p % EVTCHNS_PER_BUCKET);
 }
 
-- 
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 v8 2/9] x86/vioapic: block speculative out-of-bound accesses

[Norbert Manthey]

When interacting with io apic, a guest can specify values that are used
as index to structures, and whose values are not compared against
upper bounds to prevent speculative out-of-bound accesses. This change
prevents these speculative accesses.

Furthermore, variables are initialized and the compiler is asked to not
optimized these initializations, as the uninitialized variables might be
used in a speculative out-of-bound access. Out of the four initialized
variables, two are potentially problematic, namely ones in the functions
vioapic_irq_positive_edge and vioapic_get_trigger_mode.

As the two problematic variables are both used in the common function
gsi_vioapic, the mitigation is implemented there. As the access pattern
of the currently non-guest-controlled functions might change in the
future as well, the other variables are initialized as well.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

---

Notes:
  v8: add reviewed-by

 xen/arch/x86/hvm/vioapic.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -30,6 +30,7 @@
 #include <xen/lib.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
+#include <xen/nospec.h>
 #include <public/hvm/ioreq.h>
 #include <asm/hvm/io.h>
 #include <asm/hvm/vpic.h>
@@ -66,6 +67,12 @@ static struct hvm_vioapic *gsi_vioapic(const struct domain *d,
 {
     unsigned int i;
 
+    /*
+     * Make sure the compiler does not optimize away the initialization done by
+     * callers
+     */
+    OPTIMIZER_HIDE_VAR(*pin);
+
     for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
     {
         struct hvm_vioapic *vioapic = domain_vioapic(d, i);
@@ -117,7 +124,8 @@ static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic)
             break;
         }
 
-        redir_content = vioapic->redirtbl[redir_index].bits;
+        redir_content = vioapic->redirtbl[array_index_nospec(redir_index,
+                                                       vioapic->nr_pins)].bits;
         result = (vioapic->ioregsel & 1) ? (redir_content >> 32)
                                          : redir_content;
         break;
@@ -212,7 +220,15 @@ static void vioapic_write_redirent(
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
     union vioapic_redir_entry *pent, ent;
     int unmasked = 0;
-    unsigned int gsi = vioapic->base_gsi + idx;
+    unsigned int gsi;
+
+    /* Callers of this function should make sure idx is bounded appropriately */
+    ASSERT(idx < vioapic->nr_pins);
+
+    /* Make sure no out-of-bounds value for idx can be used */
+    idx = array_index_nospec(idx, vioapic->nr_pins);
+
+    gsi = vioapic->base_gsi + idx;
 
     spin_lock(&d->arch.hvm.irq_lock);
 
@@ -467,7 +483,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
 
 void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
 {
-    unsigned int pin;
+    unsigned int pin = 0; /* See gsi_vioapic */
     struct hvm_vioapic *vioapic = gsi_vioapic(d, irq, &pin);
     union vioapic_redir_entry *ent;
 
@@ -542,7 +558,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
 
 int vioapic_get_mask(const struct domain *d, unsigned int gsi)
 {
-    unsigned int pin;
+    unsigned int pin = 0; /* See gsi_vioapic */
     const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin);
 
     if ( !vioapic )
@@ -553,7 +569,7 @@ int vioapic_get_mask(const struct domain *d, unsigned int gsi)
 
 int vioapic_get_vector(const struct domain *d, unsigned int gsi)
 {
-    unsigned int pin;
+    unsigned int pin = 0; /* See gsi_vioapic */
     const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin);
 
     if ( !vioapic )
@@ -564,7 +580,7 @@ int vioapic_get_vector(const struct domain *d, unsigned int gsi)
 
 int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
 {
-    unsigned int pin;
+    unsigned int pin = 0; /* See gsi_vioapic */
     const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin);
 
     if ( !vioapic )
-- 
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 v8 3/9] spec: add l1tf-barrier

[Norbert Manthey]

To control the runtime behavior on L1TF vulnerable platforms better, the
command line option l1tf-barrier is introduced. This option controls
whether on vulnerable x86 platforms the lfence instruction is used to
prevent speculative execution from bypassing the evaluation of
conditionals that are protected with the evaluate_nospec macro.

By now, Xen is capable of identifying L1TF vulnerable hardware. However,
this information cannot be used for alternative patching, as a CPU feature
is required. To control alternative patching with the command line option,
a new x86 feature "X86_FEATURE_SC_L1TF_VULN" is introduced. This feature
is used to patch the lfence instruction into the arch_barrier_nospec_true
function. The feature is enabled only if L1TF vulnerable hardware is
detected and the command line option does not prevent using this feature.

The status of hyperthreading is considered when automatically enabling
adding the lfence instruction. Since platforms without hyperthreading can
still be vulnerable to L1TF in case the L1 cache is not flushed properly,
the additional lfence instructions are patched in if either hyperthreading
is enabled, or L1 cache flushing is missing.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

---

Notes:
  v8: add reviewed-by
      drop == 0 and exchange != 0 with negation

 docs/misc/xen-command-line.pandoc | 14 ++++++++++----
 xen/arch/x86/spec_ctrl.c          | 17 +++++++++++++++--
 xen/include/asm-x86/cpufeatures.h |  1 +
 xen/include/asm-x86/spec_ctrl.h   |  1 +
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -483,9 +483,9 @@ accounting for hardware capabilities as enumerated via CPUID.
 
 Currently accepted:
 
-The Speculation Control hardware features `ibrsb`, `stibp`, `ibpb`,
-`l1d-flush` and `ssbd` are used by default if available and applicable.  They can
-be ignored, e.g. `no-ibrsb`, at which point Xen won't use them itself, and
+The Speculation Control hardware features `ibrsb`, `stibp`, `ibpb`, `l1d-flush`,
+`l1tf-barrier` and `ssbd` are used by default if available and applicable.  They
+can be ignored, e.g. `no-ibrsb`, at which point Xen won't use them itself, and
 won't offer them to guests.
 
 ### cpuid_mask_cpu
@@ -1896,7 +1896,7 @@ By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`).
 ### spec-ctrl (x86)
 > `= List of [ <bool>, xen=<bool>, {pv,hvm,msr-sc,rsb}=<bool>,
 >              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,eager-fpu,
->              l1d-flush}=<bool> ]`
+>              l1d-flush,l1tf-barrier}=<bool> ]`
 
 Controls for speculative execution sidechannel mitigations.  By default, Xen
 will pick the most appropriate mitigations based on compiled in support,
@@ -1962,6 +1962,12 @@ Irrespective of Xen's setting, the feature is virtualised for HVM guests to
 use.  By default, Xen will enable this mitigation on hardware believed to be
 vulnerable to L1TF.
 
+On hardware vulnerable to L1TF, the `l1tf-barrier=` option can be used to force
+or prevent Xen from protecting evaluations inside the hypervisor with a barrier
+instruction to not load potentially secret information into L1 cache.  By
+default, Xen will enable this mitigation on hardware believed to be vulnerable
+to L1TF.
+
 ### sync_console
 > `= <boolean>`
 
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -21,6 +21,7 @@
 #include <xen/lib.h>
 #include <xen/warning.h>
 
+#include <asm/cpuid.h>
 #include <asm/microcode.h>
 #include <asm/msr.h>
 #include <asm/processor.h>
@@ -50,6 +51,7 @@ bool __read_mostly opt_ibpb = true;
 bool __read_mostly opt_ssbd = false;
 int8_t __read_mostly opt_eager_fpu = -1;
 int8_t __read_mostly opt_l1d_flush = -1;
+int8_t __read_mostly opt_l1tf_barrier = -1;
 
 bool __initdata bsp_delay_spec_ctrl;
 uint8_t __read_mostly default_xen_spec_ctrl;
@@ -91,6 +93,8 @@ static int __init parse_spec_ctrl(const char *s)
             if ( opt_pv_l1tf_domu < 0 )
                 opt_pv_l1tf_domu = 0;
 
+            opt_l1tf_barrier = 0;
+
         disable_common:
             opt_rsb_pv = false;
             opt_rsb_hvm = false;
@@ -157,6 +161,8 @@ static int __init parse_spec_ctrl(const char *s)
             opt_eager_fpu = val;
         else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
             opt_l1d_flush = val;
+        else if ( (val = parse_boolean("l1tf-barrier", s, ss)) >= 0 )
+            opt_l1tf_barrier = val;
         else
             rc = -EINVAL;
 
@@ -248,7 +254,7 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
                "\n");
 
     /* Settings for Xen's protection, irrespective of guests. */
-    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s%s\n",
+    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s%s%s\n",
            thunk == THUNK_NONE      ? "N/A" :
            thunk == THUNK_RETPOLINE ? "RETPOLINE" :
            thunk == THUNK_LFENCE    ? "LFENCE" :
@@ -258,7 +264,8 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            !boot_cpu_has(X86_FEATURE_SSBD)           ? "" :
            (default_xen_spec_ctrl & SPEC_CTRL_SSBD)  ? " SSBD+" : " SSBD-",
            opt_ibpb                                  ? " IBPB"  : "",
-           opt_l1d_flush                             ? " L1D_FLUSH" : "");
+           opt_l1d_flush                             ? " L1D_FLUSH" : "",
+           opt_l1tf_barrier                          ? " L1TF_BARRIER" : "");
 
     /* L1TF diagnostics, printed if vulnerable or PV shadowing is in use. */
     if ( cpu_has_bug_l1tf || opt_pv_l1tf_hwdom || opt_pv_l1tf_domu )
@@ -842,6 +849,12 @@ void __init init_speculation_mitigations(void)
     else if ( opt_l1d_flush == -1 )
         opt_l1d_flush = cpu_has_bug_l1tf && !(caps & ARCH_CAPS_SKIP_L1DFL);
 
+    /* By default, enable L1TF_VULN on L1TF-vulnerable hardware */
+    if ( opt_l1tf_barrier == -1 )
+        opt_l1tf_barrier = cpu_has_bug_l1tf && (opt_smt || !opt_l1d_flush);
+    if ( opt_l1tf_barrier > 0 )
+        setup_force_cpu_cap(X86_FEATURE_SC_L1TF_VULN);
+
     /*
      * We do not disable HT by default on affected hardware.
      *
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -25,6 +25,7 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPINTS+0)*32+11) /* SMAP gets used by Xen it
 XEN_CPUFEATURE(LFENCE_DISPATCH, (FSCAPINTS+0)*32+12) /* lfence set as Dispatch Serialising */
 XEN_CPUFEATURE(IND_THUNK_LFENCE,(FSCAPINTS+0)*32+13) /* Use IND_THUNK_LFENCE */
 XEN_CPUFEATURE(IND_THUNK_JMP,   (FSCAPINTS+0)*32+14) /* Use IND_THUNK_JMP */
+XEN_CPUFEATURE(SC_L1TF_VULN,    (FSCAPINTS+0)*32+15) /* L1TF protection required */
 XEN_CPUFEATURE(SC_MSR_PV,       (FSCAPINTS+0)*32+16) /* MSR_SPEC_CTRL used by Xen for PV */
 XEN_CPUFEATURE(SC_MSR_HVM,      (FSCAPINTS+0)*32+17) /* MSR_SPEC_CTRL used by Xen for HVM */
 XEN_CPUFEATURE(SC_RSB_PV,       (FSCAPINTS+0)*32+18) /* RSB overwrite needed for PV */
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -37,6 +37,7 @@ extern bool opt_ibpb;
 extern bool opt_ssbd;
 extern int8_t opt_eager_fpu;
 extern int8_t opt_l1d_flush;
+extern int8_t opt_l1tf_barrier;
 
 extern bool bsp_delay_spec_ctrl;
 extern uint8_t default_xen_spec_ctrl;
-- 
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 v8 4/9] nospec: introduce evaluate_nospec

[Norbert Manthey]

Since the L1TF vulnerability of Intel CPUs, loading hypervisor data into
L1 cache is problematic, because when hyperthreading is used as well, a
guest running on the sibling core can leak this potentially secret data.

To prevent these speculative accesses, we block speculation after
accessing the domain property field by adding lfence instructions. This
way, the CPU continues executing and loading data only once the condition
is actually evaluated.

As the macros are typically used in if statements, the lfence has to come
in a compatible way. Therefore, a function that returns true after an
lfence instruction is introduced. To protect both branches after a
conditional, an lfence instruction has to be added for the two branches.
To be able to block speculation after several evaluations, the generic
barrier macro block_speculation is also introduced.

As the L1TF vulnerability is only present on the x86 architecture, there is
no need to add protection for other architectures. Hence, the introduced
macros are defined but empty.

On the x86 architecture, by default, the lfence instruction is not present
either. Only when a L1TF vulnerable platform is detected, the lfence
instruction is patched in via alternative patching. Similarly, PV guests
are protected wrt L1TF by default, so that the protection is furthermore
disabled in case HVM is exclueded via the build configuration.

Introducing the lfence instructions catches a lot of potential leaks with
a simple unintrusive code change. During performance testing, we did not
notice performance effects.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Acked-by: Julien Grall <julien.grall@arm.com>
---

Notes:
  v8: add acked-by
      replace macros with inline functions (ARM)
      replace macros with always_inline functions (x86)

 xen/include/asm-arm/nospec.h | 25 ++++++++++++++++++++++++
 xen/include/asm-x86/nospec.h | 45 ++++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/nospec.h     |  1 +
 3 files changed, 71 insertions(+)
 create mode 100644 xen/include/asm-arm/nospec.h
 create mode 100644 xen/include/asm-x86/nospec.h

diff --git a/xen/include/asm-arm/nospec.h b/xen/include/asm-arm/nospec.h
new file mode 100644
--- /dev/null
+++ b/xen/include/asm-arm/nospec.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. */
+
+#ifndef _ASM_ARM_NOSPEC_H
+#define _ASM_ARM_NOSPEC_H
+
+static inline bool evaluate_nospec(bool condition)
+{
+  return condition;
+}
+
+static inline void block_speculation(void)
+{
+}
+
+#endif /* _ASM_ARM_NOSPEC_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/nospec.h b/xen/include/asm-x86/nospec.h
new file mode 100644
--- /dev/null
+++ b/xen/include/asm-x86/nospec.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. */
+
+#ifndef _ASM_X86_NOSPEC_H
+#define _ASM_X86_NOSPEC_H
+
+#include <asm/alternative.h>
+
+/* Allow to insert a read memory barrier into conditionals */
+static always_inline bool barrier_nospec_true(void)
+{
+#ifdef CONFIG_HVM
+    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
+#endif
+    return true;
+}
+
+/* Allow to protect evaluation of conditionasl with respect to speculation */
+static always_inline bool evaluate_nospec(bool condition)
+{
+#ifdef CONFIG_HVM
+    return (condition) ? barrier_nospec_true() : !barrier_nospec_true();
+#else
+    return condition;
+#endif
+
+}
+
+/* Allow to block speculative execution in generic code */
+// #define block_speculation() ((void)barrier_nospec_true())
+static always_inline void block_speculation(void)
+{
+    (void)barrier_nospec_true();
+}
+
+#endif /* _ASM_X86_NOSPEC_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -8,6 +8,7 @@
 #define XEN_NOSPEC_H
 
 #include <asm/system.h>
+#include <asm/nospec.h>
 
 /**
  * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise
-- 
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 v8 5/9] is_control_domain: block speculation

[Norbert Manthey]

Checks of domain properties, such as is_hardware_domain or is_hvm_domain,
might be bypassed by speculatively executing these instructions. A reason
for bypassing these checks is that these macros access the domain
structure via a pointer, and check a certain field. Since this memory
access is slow, the CPU assumes a returned value and continues the
execution.

In case an is_control_domain check is bypassed, for example during a
hypercall, data that should only be accessible by the control domain could
be loaded into the cache.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Acked-by: Jan Beulich <jbeulich@suse.com>

---

Notes:
  v8: added acked-by

 xen/include/xen/sched.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -913,10 +913,10 @@ void watchdog_domain_destroy(struct domain *d);
  *    (that is, this would not be suitable for a driver domain)
  *  - There is never a reason to deny the hardware domain access to this
  */
-#define is_hardware_domain(_d) ((_d) == hardware_domain)
+#define is_hardware_domain(_d) evaluate_nospec((_d) == hardware_domain)
 
 /* This check is for functionality specific to a control domain */
-#define is_control_domain(_d) ((_d)->is_privileged)
+#define is_control_domain(_d) evaluate_nospec((_d)->is_privileged)
 
 #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist))
 
-- 
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 v8 6/9] is_hvm/pv_domain: block speculation

[Norbert Manthey]

When checking for being an hvm domain, or PV domain, we have to make
sure that speculation cannot bypass that check, and eventually access
data that should not end up in cache for the current domain type.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Acked-by: Jan Beulich <jbeulich@suse.com>

---

Notes:
  v8: added acked by

 xen/include/xen/sched.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -922,7 +922,8 @@ void watchdog_domain_destroy(struct domain *d);
 
 static inline bool is_pv_domain(const struct domain *d)
 {
-    return IS_ENABLED(CONFIG_PV) ? d->guest_type == guest_type_pv : false;
+    return IS_ENABLED(CONFIG_PV)
+           ? evaluate_nospec(d->guest_type == guest_type_pv) : false;
 }
 
 static inline bool is_pv_vcpu(const struct vcpu *v)
@@ -953,7 +954,8 @@ static inline bool is_pv_64bit_vcpu(const struct vcpu *v)
 #endif
 static inline bool is_hvm_domain(const struct domain *d)
 {
-    return IS_ENABLED(CONFIG_HVM) ? d->guest_type == guest_type_hvm : false;
+    return IS_ENABLED(CONFIG_HVM)
+           ? evaluate_nospec(d->guest_type == guest_type_hvm) : false;
 }
 
 static inline bool is_hvm_vcpu(const struct vcpu *v)
-- 
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 v8 7/9] common/memory: block speculative out-of-bound accesses

[Norbert Manthey]

The get_page_from_gfn method returns a pointer to a page that belongs
to a gfn. Before returning the pointer, the gfn is checked for being
valid. Under speculation, these checks can be bypassed, so that
the function get_page is still executed partially. Consequently, the
function page_get_owner_and_reference might be executed partially as
well. In this function, the computed pointer is accessed, resulting in
a speculative out-of-bound address load. As the gfn can be controlled by
a guest, this access is problematic.

To mitigate the root cause, an lfence instruction is added via the
evaluate_nospec macro. To make the protection generic, we do not
introduce the lfence instruction for this single check, but add it to
the mfn_valid function. This way, other potentially problematic accesses
are protected as well.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Acked-by: Jan Beulich <jbeulich@suse.com>

---

Notes:
  v8: added acked by

 xen/common/pdx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/common/pdx.c b/xen/common/pdx.c
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -18,6 +18,7 @@
 #include <xen/init.h>
 #include <xen/mm.h>
 #include <xen/bitops.h>
+#include <xen/nospec.h>
 
 /* Parameters for PFN/MADDR compression. */
 unsigned long __read_mostly max_pdx;
@@ -33,8 +34,9 @@ unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(
 
 bool __mfn_valid(unsigned long mfn)
 {
-    return likely(mfn < max_page) &&
-           likely(!(mfn & pfn_hole_mask)) &&
+    if ( unlikely(evaluate_nospec(mfn >= max_page)) )
+        return false;
+    return likely(!(mfn & pfn_hole_mask)) &&
            likely(test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT,
                            pdx_group_valid));
 }
-- 
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 v8 8/9] x86/hvm: add nospec to hvmop param

[Norbert Manthey]

The params array in hvm can be accessed with get and set functions.
As the index is guest controlled, make sure no out-of-bound accesses
can be performed.

As we cannot influence how future compilers might modify the
instructions that enforce the bounds, we furthermore block speculation,
so that the update is visible in the architectural state.

This is part of the speculative hardening effort.

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

---

Notes:
  v8: drop a.index update before block_speculation
      improve comments

 xen/arch/x86/hvm/hvm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4109,6 +4109,11 @@ static int hvmop_set_param(
     if ( a.index >= HVM_NR_PARAMS )
         return -EINVAL;
 
+    /*
+     * Make sure the above bound check is not bypassed during speculation.
+     */
+    block_speculation();
+
     d = rcu_lock_domain_by_any_id(a.domid);
     if ( d == NULL )
         return -ESRCH;
@@ -4375,6 +4380,11 @@ static int hvmop_get_param(
     if ( a.index >= HVM_NR_PARAMS )
         return -EINVAL;
 
+    /*
+     * Make sure the above bound check is not bypassed during speculation.
+     */
+    block_speculation();
+
     d = rcu_lock_domain_by_any_id(a.domid);
     if ( d == NULL )
         return -ESRCH;
-- 
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 v8 9/9] common/grant_table: block speculative out-of-bound accesses

[Norbert Manthey]

Guests can issue grant table operations and provide guest controlled
data to them. This data is also used for memory loads. To avoid
speculative out-of-bound accesses, we use the array_index_nospec macro
where applicable. However, there are also memory accesses that cannot
be protected by a single array protection, or multiple accesses in a
row. To protect these, a nospec barrier is placed between the actual
range check and the access via the block_speculation macro.

As different versions of grant tables use structures of different size,
and the status is encoded in an array for version 2, speculative
execution might perform out-of-bound accesses of version 2 while
the table is actually using version 1. Hence, speculation is prevented
when accessing memory based on the grant table version.

This is part of the speculative hardening effort.

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

---

Notes:
  v8: fix style in shared_entry_header switch statement and comments
      add block_speculation and assert_unreachable to shared_entry_header
      add block_speculation and assert_unreachable to nr_grant_entries
      drop block_speculation if shared_entry_header is called
      rename gt_nr_grant_entries into nr_ents

 xen/common/grant_table.c | 89 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 20 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;
+    }
+
+    block_speculation();
+    ASSERT_UNREACHABLE();
+
+    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
     }
 
+    block_speculation();
+    ASSERT_UNREACHABLE();
+
     return 0;
 }
 
@@ -963,9 +988,13 @@ 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 ensures the above check cannot be bypassed 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 &&
@@ -987,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;
 
@@ -1321,6 +1350,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);
 
     /*
@@ -1418,7 +1450,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;
@@ -2026,6 +2058,7 @@ gnttab_prepare_for_transfer(
         goto fail;
     }
 
+    /* This call ensures the above check cannot be bypassed speculatively */
     sha = shared_entry_header(rgt, ref);
 
     scombo.word = *(u32 *)&sha->flags;
@@ -2223,7 +2256,11 @@ gnttab_transfer(
         okay = gnttab_prepare_for_transfer(e, d, gop.ref);
         spin_lock(&e->page_alloc_lock);
 
-        if ( unlikely(!okay) || unlikely(e->is_dying) )
+        /*
+         * 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(e->is_dying) )
         {
             bool_t drop_dom_ref = !domain_adjust_tot_pages(e, -1);
 
@@ -2253,7 +2290,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);
 
@@ -2408,9 +2445,11 @@ acquire_grant_for_copy(
         PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
                  "Bad grant reference %#x\n", gref);
 
-    act = active_entry_acquire(rgt, gref);
+    /* This call ensures the above check cannot be bypassed speculatively */
     shah = shared_entry_header(rgt, gref);
-    if ( rgt->gt_version == 1 )
+    act = active_entry_acquire(rgt, gref);
+
+    if ( evaluate_nospec(rgt->gt_version == 1) )
     {
         sha2 = NULL;
         status = &shah->flags;
@@ -2826,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);
@@ -2945,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;
@@ -2969,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 )
         {
@@ -3211,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;
@@ -3223,7 +3269,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;
 
@@ -3680,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 )
@@ -3771,7 +3818,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);
 
@@ -3793,7 +3840,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);
@@ -3935,6 +3982,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");
@@ -3947,7 +3995,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

Re: [PATCH L1TF v8 4/9] nospec: introduce evaluate_nospec

[Jan Beulich]

>>> On 25.02.19 at 14:34, <nmanthey@amazon.de> wrote:
> Since the L1TF vulnerability of Intel CPUs, loading hypervisor data into
> L1 cache is problematic, because when hyperthreading is used as well, a
> guest running on the sibling core can leak this potentially secret data.
> 
> To prevent these speculative accesses, we block speculation after
> accessing the domain property field by adding lfence instructions. This
> way, the CPU continues executing and loading data only once the condition
> is actually evaluated.
> 
> As the macros are typically used in if statements, the lfence has to come

There are no macros anymore afaics.

> --- /dev/null
> +++ b/xen/include/asm-arm/nospec.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. 
> */
> +
> +#ifndef _ASM_ARM_NOSPEC_H
> +#define _ASM_ARM_NOSPEC_H
> +
> +static inline bool evaluate_nospec(bool condition)
> +{
> +  return condition;

Insufficient indentation.

> --- /dev/null
> +++ b/xen/include/asm-x86/nospec.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. */
> +
> +#ifndef _ASM_X86_NOSPEC_H
> +#define _ASM_X86_NOSPEC_H
> +
> +#include <asm/alternative.h>
> +
> +/* Allow to insert a read memory barrier into conditionals */
> +static always_inline bool barrier_nospec_true(void)
> +{
> +#ifdef CONFIG_HVM
> +    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
> +#endif
> +    return true;
> +}
> +
> +/* Allow to protect evaluation of conditionasl with respect to speculation */
> +static always_inline bool evaluate_nospec(bool condition)
> +{
> +#ifdef CONFIG_HVM
> +    return (condition) ? barrier_nospec_true() : !barrier_nospec_true();

No need for the parentheses anymore. And is the #ifdef really needed
here?

> +#else
> +    return condition;
> +#endif
> +
> +}

Stray blank line.

> +/* Allow to block speculative execution in generic code */
> +// #define block_speculation() ((void)barrier_nospec_true())

Stray leftover line.

> +static always_inline void block_speculation(void)
> +{
> +    (void)barrier_nospec_true();

No need for the cast anymore.

Jan



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

Re: [PATCH L1TF v8 8/9] x86/hvm: add nospec to hvmop param

[Jan Beulich]

>>> On 25.02.19 at 14:34, <nmanthey@amazon.de> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4109,6 +4109,11 @@ static int hvmop_set_param(
>      if ( a.index >= HVM_NR_PARAMS )
>          return -EINVAL;
>  
> +    /*
> +     * Make sure the above bound check is not bypassed during speculation.
> +     */
> +    block_speculation();
> +
>      d = rcu_lock_domain_by_any_id(a.domid);
>      if ( d == NULL )
>          return -ESRCH;
> @@ -4375,6 +4380,11 @@ static int hvmop_get_param(
>      if ( a.index >= HVM_NR_PARAMS )
>          return -EINVAL;
>  
> +    /*
> +     * Make sure the above bound check is not bypassed during speculation.
> +     */
> +    block_speculation();

Comment style (single line) in both cases. This could be done while
committing, and with it taken care of
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH L1TF v8 9/9] common/grant_table: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 25.02.19 at 14:34, <nmanthey@amazon.de> wrote:
> @@ -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
>      }
>  
> +    block_speculation();
> +    ASSERT_UNREACHABLE();
> +
>      return 0;
>  }

Personally I think the assertion should be first (also in
shared_entry_header()), but that's nothing very important to
change.

Below here, but before the next patch hunk, is _set_status(). If
you think there's no need to change its gt_version check, then I
think the commit message should (briefly) explain this.

> @@ -1418,7 +1450,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;

Just like above, below here (in the same function) is another version
check you don't adjust, and there are further ones in gnttab_grow_table(),
gnttab_setup_table(), and release_grant_for_copy().

> @@ -2408,9 +2445,11 @@ acquire_grant_for_copy(
>          PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
>                   "Bad grant reference %#x\n", gref);
>  
> -    act = active_entry_acquire(rgt, gref);
> +    /* This call ensures the above check cannot be bypassed speculatively */
>      shah = shared_entry_header(rgt, gref);
> -    if ( rgt->gt_version == 1 )
> +    act = active_entry_acquire(rgt, gref);
> +
> +    if ( evaluate_nospec(rgt->gt_version == 1) )
>      {
>          sha2 = NULL;
>          status = &shah->flags;

There's again a second version check further down in this function.

> @@ -2945,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;
> @@ -2969,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 )
>          {

What about the various version accesses in this function? And
while I think the one in gnttab_release_mappings() doesn't need
adjustment, it should (also) be mentioned in the description. The
one in gnttab_map_frame(9, otoh, looks as if it again needed
adjustment.

I would really like to ask that I (or someone else) don't need to
go through and list remaining version checks again - after all I
had done so for v6 already, and I didn't go through all of them
again for v7 assuming that you would have worked through the
entire set.

Mentioning reasons of omitted adjustments is in particular
important for people to have a reference down the road, to be
able to tell whether new version checks to add need to take one
shape or the other.

Jan



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

Re: [PATCH L1TF v8 1/9] xen/evtchn: block speculative out-of-bound accesses

[Juergen Gross]

On 25/02/2019 14:34, Norbert Manthey wrote:
> Guests can issue event channel interaction with guest specified data.
> To avoid speculative out-of-bound accesses, we use the nospec macros,
> or the domain_vcpu function. Where appropriate, we use the vcpu_id of
> the seleceted vcpu instead of the parameter that can be influenced by
> the guest, so that only one access needs to be protected.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH L1TF v8 2/9] x86/vioapic: block speculative out-of-bound accesses

[Juergen Gross]

On 25/02/2019 14:34, Norbert Manthey wrote:
> When interacting with io apic, a guest can specify values that are used
> as index to structures, and whose values are not compared against
> upper bounds to prevent speculative out-of-bound accesses. This change
> prevents these speculative accesses.
> 
> Furthermore, variables are initialized and the compiler is asked to not
> optimized these initializations, as the uninitialized variables might be
> used in a speculative out-of-bound access. Out of the four initialized
> variables, two are potentially problematic, namely ones in the functions
> vioapic_irq_positive_edge and vioapic_get_trigger_mode.
> 
> As the two problematic variables are both used in the common function
> gsi_vioapic, the mitigation is implemented there. As the access pattern
> of the currently non-guest-controlled functions might change in the
> future as well, the other variables are initialized as well.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH L1TF v8 3/9] spec: add l1tf-barrier

[Juergen Gross]

On 25/02/2019 14:34, Norbert Manthey wrote:
> To control the runtime behavior on L1TF vulnerable platforms better, the
> command line option l1tf-barrier is introduced. This option controls
> whether on vulnerable x86 platforms the lfence instruction is used to
> prevent speculative execution from bypassing the evaluation of
> conditionals that are protected with the evaluate_nospec macro.
> 
> By now, Xen is capable of identifying L1TF vulnerable hardware. However,
> this information cannot be used for alternative patching, as a CPU feature
> is required. To control alternative patching with the command line option,
> a new x86 feature "X86_FEATURE_SC_L1TF_VULN" is introduced. This feature
> is used to patch the lfence instruction into the arch_barrier_nospec_true
> function. The feature is enabled only if L1TF vulnerable hardware is
> detected and the command line option does not prevent using this feature.
> 
> The status of hyperthreading is considered when automatically enabling
> adding the lfence instruction. Since platforms without hyperthreading can
> still be vulnerable to L1TF in case the L1 cache is not flushed properly,
> the additional lfence instructions are patched in if either hyperthreading
> is enabled, or L1 cache flushing is missing.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH L1TF v8 4/9] nospec: introduce evaluate_nospec

[Norbert Manthey]

On 2/25/19 16:54, Jan Beulich wrote:
>>>> On 25.02.19 at 14:34, <nmanthey@amazon.de> wrote:
>> Since the L1TF vulnerability of Intel CPUs, loading hypervisor data into
>> L1 cache is problematic, because when hyperthreading is used as well, a
>> guest running on the sibling core can leak this potentially secret data.
>>
>> To prevent these speculative accesses, we block speculation after
>> accessing the domain property field by adding lfence instructions. This
>> way, the CPU continues executing and loading data only once the condition
>> is actually evaluated.
>>
>> As the macros are typically used in if statements, the lfence has to come
> There are no macros anymore afaics.
I will rephrase the commit message accordingly.
>
>> --- /dev/null
>> +++ b/xen/include/asm-arm/nospec.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. 
>> */
>> +
>> +#ifndef _ASM_ARM_NOSPEC_H
>> +#define _ASM_ARM_NOSPEC_H
>> +
>> +static inline bool evaluate_nospec(bool condition)
>> +{
>> +  return condition;
> Insufficient indentation.
Fixed to 4 spaces.
>
>> --- /dev/null
>> +++ b/xen/include/asm-x86/nospec.h
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. */
>> +
>> +#ifndef _ASM_X86_NOSPEC_H
>> +#define _ASM_X86_NOSPEC_H
>> +
>> +#include <asm/alternative.h>
>> +
>> +/* Allow to insert a read memory barrier into conditionals */
>> +static always_inline bool barrier_nospec_true(void)
>> +{
>> +#ifdef CONFIG_HVM
>> +    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
>> +#endif
>> +    return true;
>> +}
>> +
>> +/* Allow to protect evaluation of conditionasl with respect to speculation */
>> +static always_inline bool evaluate_nospec(bool condition)
>> +{
>> +#ifdef CONFIG_HVM
>> +    return (condition) ? barrier_nospec_true() : !barrier_nospec_true();
> No need for the parentheses anymore. And is the #ifdef really needed
> here?
The #ifdef is not needed here, as the compiler just drops
barrier_nospec_true in case CONFIG_HVM is not specified.
>
>> +#else
>> +    return condition;
>> +#endif
>> +
>> +}
> Stray blank line.
Will drop.
>
>> +/* Allow to block speculative execution in generic code */
>> +// #define block_speculation() ((void)barrier_nospec_true())
> Stray leftover line.
Will drop.
>
>> +static always_inline void block_speculation(void)
>> +{
>> +    (void)barrier_nospec_true();
> No need for the cast anymore.

Will drop.

Best,
Norbert





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 v8 8/9] x86/hvm: add nospec to hvmop param

[Norbert Manthey]

On 2/25/19 16:59, Jan Beulich wrote:
>>>> On 25.02.19 at 14:34, <nmanthey@amazon.de> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4109,6 +4109,11 @@ static int hvmop_set_param(
>>      if ( a.index >= HVM_NR_PARAMS )
>>          return -EINVAL;
>>  
>> +    /*
>> +     * Make sure the above bound check is not bypassed during speculation.
>> +     */
>> +    block_speculation();
>> +
>>      d = rcu_lock_domain_by_any_id(a.domid);
>>      if ( d == NULL )
>>          return -ESRCH;
>> @@ -4375,6 +4380,11 @@ static int hvmop_get_param(
>>      if ( a.index >= HVM_NR_PARAMS )
>>          return -EINVAL;
>>  
>> +    /*
>> +     * Make sure the above bound check is not bypassed during speculation.
>> +     */
>> +    block_speculation();
> Comment style (single line) in both cases. This could be done while
> committing, and with it taken care of
> Acked-by: Jan Beulich <jbeulich@suse.com>

I'll turn the comments into single line comments.

Best,
Norbert




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 v8 9/9] common/grant_table: block speculative out-of-bound accesses

[Norbert Manthey]

On 2/25/19 17:46, Jan Beulich wrote:
>>>> On 25.02.19 at 14:34, <nmanthey@amazon.de> wrote:
>> @@ -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
>>      }
>>  
>> +    block_speculation();
>> +    ASSERT_UNREACHABLE();
>> +
>>      return 0;
>>  }
> Personally I think the assertion should be first (also in
> shared_entry_header()), but that's nothing very important to
> change.
I will change the order.
>
> Below here, but before the next patch hunk, is _set_status(). If
> you think there's no need to change its gt_version check, then I
> think the commit message should (briefly) explain this.
>
>> @@ -1418,7 +1450,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;
> Just like above, below here (in the same function) is another version
> check you don't adjust, and there are further ones in gnttab_grow_table(),
> gnttab_setup_table(), and release_grant_for_copy().
>
>> @@ -2408,9 +2445,11 @@ acquire_grant_for_copy(
>>          PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
>>                   "Bad grant reference %#x\n", gref);
>>  
>> -    act = active_entry_acquire(rgt, gref);
>> +    /* This call ensures the above check cannot be bypassed speculatively */
>>      shah = shared_entry_header(rgt, gref);
>> -    if ( rgt->gt_version == 1 )
>> +    act = active_entry_acquire(rgt, gref);
>> +
>> +    if ( evaluate_nospec(rgt->gt_version == 1) )
>>      {
>>          sha2 = NULL;
>>          status = &shah->flags;
> There's again a second version check further down in this function.
>
>> @@ -2945,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;
>> @@ -2969,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 )
>>          {
> What about the various version accesses in this function? And
> while I think the one in gnttab_release_mappings() doesn't need
> adjustment, it should (also) be mentioned in the description. The
> one in gnttab_map_frame(9, otoh, looks as if it again needed
> adjustment.
>
> I would really like to ask that I (or someone else) don't need to
> go through and list remaining version checks again - after all I
> had done so for v6 already, and I didn't go through all of them
> again for v7 assuming that you would have worked through the
> entire set.

So, here is the annotation for all of them. Anyone that I did not
include in the list has been fixed in previous versions, or will be
fixed in the next version:

git grep -np version common/grant_table.c

common/grant_table.c:831:static int _set_status(unsigned gt_version,
common/grant_table.c:840:    if ( gt_version == 1 )

-> I do not see how out-of-bound accesses happen in the called functions
there.

common/grant_table.c=1444=unmap_common_complete(struct
gnttab_unmap_common *op)
common/grant_table.c:1469:    if ( rgt->gt_version == 1 )

-> I do not see how to be exploitable, as the shared_entry_header call
above just used an lfence.

common/grant_table.c=1761=gnttab_grow_table(struct domain *d, unsigned
int req_nr_frames)
common/grant_table.c:1800:    /* Status pages - version 2 */
common/grant_table.c:1801:    if ( gt->gt_version > 1 )

-> I do not see how out-of-bound access could happen. This calls
gnttab_populate_status_frames that allocates pages and should not touch
more memory than before

common/grant_table.c=1904=gnttab_setup_table(
common/grant_table.c:1955:          ((gt->gt_version > 1) &&

-> I do not see how an out-of-bound access is possible.

common/grant_table.c=2104=gnttab_transfer(
common/grant_table.c:2199:            e, e->grant_table->gt_version > 1
|| paging_mode_translate(e)

-> I do not see dependent out-of-bound accesses.

common/grant_table.c=2337=release_grant_for_copy(
common/grant_table.c:2354:    if ( rgt->gt_version == 1 )
common/grant_table.c=2420=acquire_grant_for_copy(
common/grant_table.c:2452:    if ( evaluate_nospec(rgt->gt_version == 1) )
common/grant_table.c:2535:        if ( rgt->gt_version != 2 ||

-> I do not see dependent out-of-bound accesses.

common/grant_table.c=2982=static long
common/grant_table.c:2983:gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t)
uop)

-> in case of a version change, both fields are touched, and potential
out-of-bound accesses can be performed once, as the memory is allocated
afterwards and not freed during domain life time.

common/grant_table.c=3618=gnttab_release_mappings(
common/grant_table.c:3657:        if ( rgt->gt_version == 1 )

-> Is only called during domain destruction.

common/grant_table.c=3809=int mem_sharing_gref_to_gfn(struct grant_table
*gt, grant_ref_t ref,
common/grant_table.c:3817:    if ( gt->gt_version < 1 )

-> The next evaluate_nospec in the code covers the relevant memory accesses.

common/grant_table.c=3966=int gnttab_get_status_frame(struct domain *d,
unsigned long idx,
common/grant_table.c:3973:    rc = (gt->gt_version == 2) ?

-> The out-of-bound access will be blocked by using block_speculation in
gnttab_get_status_frame_mfn.

common/grant_table.c=3980=static void gnttab_usage_print(struct domain *rd)
common/grant_table.c:3994:           rd->domain_id, gt->gt_version,
common/grant_table.c:4015:        if ( gt->gt_version == 1 )

-> This function cannot be triggered by a guest.

>
> Mentioning reasons of omitted adjustments is in particular
> important for people to have a reference down the road, to be
> able to tell whether new version checks to add need to take one
> shape or the other.

I understand that his is important for future modifications. I will
extend the commit message with the reasons when not to block speculation
wrt the version check, i.e. cannot be triggered by the guest, does not
return to the guest, does not result in an out-of-bound access or cannot
be executed repeatedly.

Best,
Norbert

> 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 v8 9/9] common/grant_table: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 27.02.19 at 14:01, <nmanthey@amazon.de> wrote:
> On 2/25/19 17:46, Jan Beulich wrote:
>> I would really like to ask that I (or someone else) don't need to
>> go through and list remaining version checks again - after all I
>> had done so for v6 already, and I didn't go through all of them
>> again for v7 assuming that you would have worked through the
>> entire set.
> 
> So, here is the annotation for all of them. Anyone that I did not
> include in the list has been fixed in previous versions, or will be
> fixed in the next version:
> 
> git grep -np version common/grant_table.c
> 
> common/grant_table.c:831:static int _set_status(unsigned gt_version,
> common/grant_table.c:840:    if ( gt_version == 1 )
> 
> -> I do not see how out-of-bound accesses happen in the called functions
> there.

Both functions get shah passed into them, which may point to the
other version's layout. Earlier fences don't help speculation on this
conditional.

> common/grant_table.c=1444=unmap_common_complete(struct
> gnttab_unmap_common *op)
> common/grant_table.c:1469:    if ( rgt->gt_version == 1 )
> 
> -> I do not see how to be exploitable, as the shared_entry_header call
> above just used an lfence.

Again, earlier fences don't suppress subsequent speculation on
an independent conditional.

> common/grant_table.c=1761=gnttab_grow_table(struct domain *d, unsigned
> int req_nr_frames)
> common/grant_table.c:1800:    /* Status pages - version 2 */
> common/grant_table.c:1801:    if ( gt->gt_version > 1 )
> 
> -> I do not see how out-of-bound access could happen. This calls
> gnttab_populate_status_frames that allocates pages and should not touch
> more memory than before

We've been talking about the speculation window being perhaps
hundreds of insns. It may be purely theoretical, but speculation all
the way through alloc_xenheap_page() would lead to a speculative
store to gt->status[]. _Right now_ that array gets allocated in
grant_table_init(), but I can't see why that couldn't be moved to
gnttab_set_version(), so the access above could be a latent issue.

I'll skip going into details of further ones, assuming that some of
them follow patterns above.

Jan



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

Re: [PATCH L1TF v8 9/9] common/grant_table: block speculative out-of-bound accesses

[Norbert Manthey]

On 2/28/19 11:00, Jan Beulich wrote:
>>>> On 27.02.19 at 14:01, <nmanthey@amazon.de> wrote:
>> On 2/25/19 17:46, Jan Beulich wrote:
>>> I would really like to ask that I (or someone else) don't need to
>>> go through and list remaining version checks again - after all I
>>> had done so for v6 already, and I didn't go through all of them
>>> again for v7 assuming that you would have worked through the
>>> entire set.
>> So, here is the annotation for all of them. Anyone that I did not
>> include in the list has been fixed in previous versions, or will be
>> fixed in the next version:
>>
>> git grep -np version common/grant_table.c
>>
>> common/grant_table.c:831:static int _set_status(unsigned gt_version,
>> common/grant_table.c:840:    if ( gt_version == 1 )
>>
>> -> I do not see how out-of-bound accesses happen in the called functions
>> there.
> Both functions get shah passed into them, which may point to the
> other version's layout. Earlier fences don't help speculation on this
> conditional.
Whenever this function is called, the shah field has the same structure
for both versions. The v2 grant entry has a plain header hdr, which is
used here, and which is forwarded to the set_status method. Another
property that we never talked about here is that we only care about
cross cache-line accesses. As soon as any byte is touched on a cache
line, the whole line is brought into the cache. As the code around the
set_status method already pulls in the corresponding cache line, that
code looks okay to me from a L1TF perspective.
>
>> common/grant_table.c=1444=unmap_common_complete(struct
>> gnttab_unmap_common *op)
>> common/grant_table.c:1469:    if ( rgt->gt_version == 1 )
>>
>> -> I do not see how to be exploitable, as the shared_entry_header call
>> above just used an lfence.
> Again, earlier fences don't suppress subsequent speculation on
> an independent conditional.

Yes, agreed. However, in the current version, the array is allocated
already for the one branch, and the flags are in the cache for both
versions as well. The above evaluate_nospec that protects the evaluation
of the variable "done" above prevents out-of-bound values for ref. Now,
if you want me to be prepared for the future where people might move the
allocation of the status array from the generic code to the v2 specific
code, I would have to add another lfence here today already.

>
>> common/grant_table.c=1761=gnttab_grow_table(struct domain *d, unsigned
>> int req_nr_frames)
>> common/grant_table.c:1800:    /* Status pages - version 2 */
>> common/grant_table.c:1801:    if ( gt->gt_version > 1 )
>>
>> -> I do not see how out-of-bound access could happen. This calls
>> gnttab_populate_status_frames that allocates pages and should not touch
>> more memory than before
> We've been talking about the speculation window being perhaps
> hundreds of insns. It may be purely theoretical, but speculation all
> the way through alloc_xenheap_page() would lead to a speculative
> store to gt->status[]. _Right now_ that array gets allocated in
> grant_table_init(), but I can't see why that couldn't be moved to
> gnttab_set_version(), so the access above could be a latent issue.

I agree that this might be a latent issue, and in case people modify the
code, the effect wrt L1TF mitigation might have to be taken into
account. While we have a specific problematic code snippet here, that
statement is true for all the other mitigations as well. Since I do not
see a problem with the code today, and I do not like to pay the penalty
of the lfence to protect against something that might not happen in the
future, I would not add the lfence here.

Best,
Norbert

>
> I'll skip going into details of further ones, assuming that some of
> them follow patterns above.
>
> 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 v8 9/9] common/grant_table: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 04.03.19 at 09:15, <nmanthey@amazon.de> wrote:
> On 2/28/19 11:00, Jan Beulich wrote:
>>>>> On 27.02.19 at 14:01, <nmanthey@amazon.de> wrote:
>>> On 2/25/19 17:46, Jan Beulich wrote:
>>>> I would really like to ask that I (or someone else) don't need to
>>>> go through and list remaining version checks again - after all I
>>>> had done so for v6 already, and I didn't go through all of them
>>>> again for v7 assuming that you would have worked through the
>>>> entire set.
>>> So, here is the annotation for all of them. Anyone that I did not
>>> include in the list has been fixed in previous versions, or will be
>>> fixed in the next version:
>>>
>>> git grep -np version common/grant_table.c
>>>
>>> common/grant_table.c:831:static int _set_status(unsigned gt_version,
>>> common/grant_table.c:840:    if ( gt_version == 1 )
>>>
>>> -> I do not see how out-of-bound accesses happen in the called functions
>>> there.
>> Both functions get shah passed into them, which may point to the
>> other version's layout. Earlier fences don't help speculation on this
>> conditional.
> Whenever this function is called, the shah field has the same structure
> for both versions.

But it does matter how it was obtained, doesn't it? There's
shared_raw[] involved here once again. Hence we're safe here
only if the caller has already suitably guarded against speculation.
Which may indeed be the case, but which then needs to be called
out in the description.

> The v2 grant entry has a plain header hdr, which is
> used here, and which is forwarded to the set_status method. Another
> property that we never talked about here is that we only care about
> cross cache-line accesses. As soon as any byte is touched on a cache
> line, the whole line is brought into the cache. As the code around the
> set_status method already pulls in the corresponding cache line, that
> code looks okay to me from a L1TF perspective.

So here as well as for the other points - if you decide to omit guarding,
please explain why in the commit message.

Jan



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