SpectreV1+L1TF Patch Series v7

SpectreV1+L1TF Patch Series v7

[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), namely to avoid
touching memory from the hypervisor speculatively that would not be touched
without speculation. 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 changes between v6 and v7 of this series are preferring the lfence
instruction to block speculation over updating variables. Furthermore, when
patching in the lfence instruction automatically, the setting of smt and l1d
flushing are taken into account.

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 SpectreV1+L1TF v7 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>

---
Notes:
  v7: mention speculative hardening in commit message
      explain preferred use of internal data in commit message
      drop update in set_global_virq_handler

 xen/common/event_channel.c | 29 ++++++++++++++++++-----------
 xen/common/event_fifo.c    | 13 ++++++++++---
 xen/include/xen/event.h    |  5 +++--
 3 files changed, 31 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);
@@ -813,6 +817,7 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
 
     if (virq >= NR_VIRQS)
         return -EINVAL;
+
     if (!virq_is_global(virq))
         return -EINVAL;
 
@@ -930,8 +935,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 +962,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

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

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
> @@ -813,6 +817,7 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
>  
>      if (virq >= NR_VIRQS)
>          return -EINVAL;
> +
>      if (!virq_is_global(virq))
>          return -EINVAL;
>  

Stray (but benign) change. Easy enough to take out while committing.
Without this:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Btw, it would have been nice if you had also dropped the somewhat
misleading SpectreV1 from the subject line tags of the series.

Jan



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

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

[Norbert Manthey]

On 2/22/19 14:00, Jan Beulich wrote:
>>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
>> @@ -813,6 +817,7 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
>>  
>>      if (virq >= NR_VIRQS)
>>          return -EINVAL;
>> +
>>      if (!virq_is_global(virq))
>>          return -EINVAL;
>>  
> Stray (but benign) change. Easy enough to take out while committing.
> Without this:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Btw, it would have been nice if you had also dropped the somewhat
> misleading SpectreV1 from the subject line tags of the series.

I'll do that for the next round.

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 SpectreV1+L1TF v7 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>

---

Notes:
  v7: mention speculative hardening in commit message
      fix comment typo
      drop 'guest controlled' from commit message

 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

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

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> 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>



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

[PATCH SpectreV1+L1TF v7 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>

---

Notes:
  v7: mention speculative hardening in commit message
      consider smt and l1dflush in auto setting for l1tf-barrier

 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 != 0 || opt_l1d_flush == 0);
+    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

Re: [PATCH SpectreV1+L1TF v7 3/9] spec: add l1tf-barrier

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> 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>
with one remark:

> @@ -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 != 0 || opt_l1d_flush == 0);

We commonly omit "!= 0" and use ! instead of "== 0". If I end
up committing this, I may take the liberty of changing these.

Jan



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

[PATCH SpectreV1+L1TF v7 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>
---

Notes:
  v7: mention speculative hardening in commit messate
      drop system.h include
      drop arch prefix
      add outer brackets in block_speculation

 xen/include/asm-arm/nospec.h | 20 ++++++++++++++++++++
 xen/include/asm-x86/nospec.h | 38 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/nospec.h     |  1 +
 3 files changed, 59 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,20 @@
+/* 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
+
+#define evaluate_nospec(condition) (condition)
+
+#define block_speculation()
+
+#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,38 @@
+/* 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 */
+#ifdef CONFIG_HVM
+#define evaluate_nospec(condition)                                         \
+    ((condition) ? barrier_nospec_true() : !barrier_nospec_true())
+#else
+#define evaluate_nospec(condition) (condition)
+#endif
+
+/* Allow to block speculative execution in generic code */
+#define block_speculation() ((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

Re: [PATCH SpectreV1+L1TF v7 4/9] nospec: introduce evaluate_nospec

[Julien Grall]

Hi Norbert,

On 21/02/2019 08:16, Norbert Manthey wrote:
> 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,20 @@
> +/* 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
> +
> +#define evaluate_nospec(condition) (condition)
> +
> +#define block_speculation()

NIT: Could you turn this to a static inline?

With that for the Arm code:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

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

Re: [PATCH SpectreV1+L1TF v7 4/9] nospec: introduce evaluate_nospec

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
> --- /dev/null
> +++ b/xen/include/asm-x86/nospec.h
> @@ -0,0 +1,38 @@
> +/* 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 */
> +#ifdef CONFIG_HVM
> +#define evaluate_nospec(condition)                                         \
> +    ((condition) ? barrier_nospec_true() : !barrier_nospec_true())
> +#else
> +#define evaluate_nospec(condition) (condition)
> +#endif
> +
> +/* Allow to block speculative execution in generic code */
> +#define block_speculation() ((void)barrier_nospec_true())

Seeing Julien's request for switching to inline functions on the Arm side,
is there anything preventing these two to become inline functions too?

Jan



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

Re: [PATCH SpectreV1+L1TF v7 4/9] nospec: introduce evaluate_nospec

[Norbert Manthey]

On 2/22/19 14:17, Jan Beulich wrote:
>>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
>> --- /dev/null
>> +++ b/xen/include/asm-x86/nospec.h
>> @@ -0,0 +1,38 @@
>> +/* 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 */
>> +#ifdef CONFIG_HVM
>> +#define evaluate_nospec(condition)                                         \
>> +    ((condition) ? barrier_nospec_true() : !barrier_nospec_true())
>> +#else
>> +#define evaluate_nospec(condition) (condition)
>> +#endif
>> +
>> +/* Allow to block speculative execution in generic code */
>> +#define block_speculation() ((void)barrier_nospec_true())
> Seeing Julien's request for switching to inline functions on the Arm side,
> is there anything preventing these two to become inline functions too?

No, there is nothing preventing using functions here. I mostly copied
the macro style from other nospec constructs. I tested the generated
binary for the functions being always_inline, and the result is the same
as when using macros. I will turn the macros into functions that use the
always_inline keyword, as there have been problems with the
barrier_nospec_true function when not using always_inline.

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 SpectreV1+L1TF v7 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>

---

Notes:
  v7: mention speculative hardening in commit message

 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

Re: [PATCH SpectreV1+L1TF v7 5/9] is_control_domain: block speculation

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
> 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>



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

[PATCH SpectreV1+L1TF v7 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>

---

Notes:
  v7: mention speculative hardening in commit message

 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

Re: [PATCH SpectreV1+L1TF v7 6/9] is_hvm/pv_domain: block speculation

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
> 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>



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

[PATCH SpectreV1+L1TF v7 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>

---

Notes:
  v7: mention speculative hardening in commit message
      split return statement into bound check and remainder

 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

Re: [PATCH SpectreV1+L1TF v7 7/9] common/memory: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
> 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>



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

[PATCH SpectreV1+L1TF v7 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:
  v7: add speculative hardening to commit message
      add lfence to a.index updates (other parts of that commit have
          been committed already)

 xen/arch/x86/hvm/hvm.c | 14 ++++++++++++++
 1 file changed, 14 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,13 @@ static int hvmop_set_param(
     if ( a.index >= HVM_NR_PARAMS )
         return -EINVAL;
 
+    /*
+     * Make sure the guest controlled value a.index is bounded even during
+     * speculative execution.
+     */
+    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
+    block_speculation();
+
     d = rcu_lock_domain_by_any_id(a.domid);
     if ( d == NULL )
         return -ESRCH;
@@ -4375,6 +4382,13 @@ static int hvmop_get_param(
     if ( a.index >= HVM_NR_PARAMS )
         return -EINVAL;
 
+    /*
+     * Make sure the guest controlled value a.index is bounded even during
+     * speculative execution.
+     */
+    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
+    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

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

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4109,6 +4109,13 @@ static int hvmop_set_param(
>      if ( a.index >= HVM_NR_PARAMS )
>          return -EINVAL;
>  
> +    /*
> +     * Make sure the guest controlled value a.index is bounded even during
> +     * speculative execution.
> +     */
> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
> +    block_speculation();
> +
>      d = rcu_lock_domain_by_any_id(a.domid);
>      if ( d == NULL )
>          return -ESRCH;
> @@ -4375,6 +4382,13 @@ static int hvmop_get_param(
>      if ( a.index >= HVM_NR_PARAMS )
>          return -EINVAL;
>  
> +    /*
> +     * Make sure the guest controlled value a.index is bounded even during
> +     * speculative execution.
> +     */
> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
> +    block_speculation();

Please can the comments briefly explain the otherwise apparently
pointless redundancy of both constructs?

Jan



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

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

[Norbert Manthey]

On 2/22/19 15:39, Jan Beulich wrote:
>>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4109,6 +4109,13 @@ static int hvmop_set_param(
>>      if ( a.index >= HVM_NR_PARAMS )
>>          return -EINVAL;
>>  
>> +    /*
>> +     * Make sure the guest controlled value a.index is bounded even during
>> +     * speculative execution.
>> +     */
>> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
>> +    block_speculation();
>> +
>>      d = rcu_lock_domain_by_any_id(a.domid);
>>      if ( d == NULL )
>>          return -ESRCH;
>> @@ -4375,6 +4382,13 @@ static int hvmop_get_param(
>>      if ( a.index >= HVM_NR_PARAMS )
>>          return -EINVAL;
>>  
>> +    /*
>> +     * Make sure the guest controlled value a.index is bounded even during
>> +     * speculative execution.
>> +     */
>> +    a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
>> +    block_speculation();
> Please can the comments briefly explain the otherwise apparently
> pointless redundancy of both constructs?

Sure, I will add some reasoning.

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 SpectreV1+L1TF v7 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:
  v7: mention speculative hardening in commit message
      add spaces for arithmetic
      introduce switch and block_speculation macro in shared_entry_header
      introduce block_speculation macro in nr_grant_entries
      use block_speculation instead of array_index_nospec(..., nr_grant_entries)
          as nr_grant_entries comes with an lfence instruction by now
      cache results of nr_grant_entries when being used in loops to avoid lfence
      use lfence in shared_entry_header to ensure bound checks

 xen/common/grant_table.c | 78 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 61 insertions(+), 17 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,18 @@ 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:
+        /* Make sure we return a value independently of speculative execution */
+        block_speculation();
         return (grant_entry_header_t*)&shared_entry_v1(t, ref);
-    else
+    case 2:
+        /* Make sure we return a value independently of speculative execution */
+        block_speculation();
         return &shared_entry_v2(t, ref).hdr;
+    }
+    return NULL;
 }
 
 /* Active grant entry - used for shadowing GTF_permit_access grants. */
@@ -634,10 +644,16 @@ 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
     }
@@ -963,9 +979,15 @@ map_grant_ref(
         PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
                  op->ref, rgt->domain->domain_id);
 
+    /* Make sure the above bound check cannot be bypassed speculatively */
+    block_speculation();
+
     act = active_entry_acquire(rgt, op->ref);
     shah = shared_entry_header(rgt, op->ref);
-    status = rgt->gt_version == 1 ? &shah->flags : &status_entry(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 +1009,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 +1343,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 +1443,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 +2051,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 +2249,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 +2283,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 +2438,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 makes sure the above check is not 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 +2858,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);
@@ -2946,6 +2981,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
     grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
     int res;
     unsigned int i;
+    unsigned int gt_nr_grant_entries;
 
     if ( copy_from_guest(&op, uop, 1) )
         return -EFAULT;
@@ -2969,7 +3005,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++ )
+    gt_nr_grant_entries = nr_grant_entries(gt);
+    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < gt_nr_grant_entries; i++ )
     {
         if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
         {
@@ -3211,6 +3248,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 +3263,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;
 
@@ -3681,12 +3721,14 @@ void grant_table_warn_active_grants(struct domain *d)
     struct active_grant_entry *act;
     grant_ref_t ref;
     unsigned int nr_active = 0;
+    unsigned int gt_nr_grant_entries;
 
 #define WARN_GRANT_MAX 10
 
     grant_read_lock(gt);
 
-    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
+    gt_nr_grant_entries = nr_grant_entries(gt);
+    for ( ref = 0; ref != gt_nr_grant_entries; ref++ )
     {
         act = active_entry_acquire(gt, ref);
         if ( !act->pin )
@@ -3771,7 +3813,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 +3835,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 +3977,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 gt_nr_grant_entries;
 
     printk("      -------- active --------       -------- shared --------\n");
     printk("[ref] localdom mfn      pin          localdom gmfn     flags\n");
@@ -3947,7 +3990,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++ )
+    gt_nr_grant_entries = nr_grant_entries(gt);
+    for ( ref = 0; ref != gt_nr_grant_entries; 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 SpectreV1+L1TF v7 9/9] common/grant_table: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
> @@ -226,10 +228,18 @@ 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:
> +        /* Make sure we return a value independently of speculative execution */
> +        block_speculation();
>          return (grant_entry_header_t*)&shared_entry_v1(t, ref);
> -    else
> +    case 2:
> +        /* Make sure we return a value independently of speculative execution */
> +        block_speculation();
>          return &shared_entry_v2(t, ref).hdr;
> +    }
> +    return NULL;
>  }

I'm not happy with the comment wording, as to me it reads ambiguously
at best. How about "Make sure the value returned is independent of
speculative execution"? I'm of course open to even better suggestions,
in particular by native speakers.

Also please add a blank line
- between the individual case blocks,
- before the final (main) return statement.

Plus please add ASSERT_UNREACHABLE() ahead of the NULL return.

> @@ -963,9 +979,15 @@ map_grant_ref(
>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>                   op->ref, rgt->domain->domain_id);
>  
> +    /* Make sure the above bound check cannot be bypassed speculatively */
> +    block_speculation();
> +
>      act = active_entry_acquire(rgt, op->ref);
>      shah = shared_entry_header(rgt, op->ref);

So shared_entry_header() now has a fence before consuming op->ref.
Is there anything wrong with swapping these two and dropping the
extra fence you add above, like you do in acquire_grant_for_copy()? All
this would seem to require is adding block_speculation() also onto the
"return NULL" path of shared_entry_header() (where it shouldn't hurt
at all).

> @@ -2946,6 +2981,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>      grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
>      int res;
>      unsigned int i;
> +    unsigned int gt_nr_grant_entries;

Rather then lengthening the name by adding a disambiguating prefix,
how about shortening it to "nr" or "nr_ents" (also elsewhere)? Also
please add onto the line declaring i instead of adding yet another line
with the same type.

> @@ -2969,7 +3005,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++ )
> +    gt_nr_grant_entries = nr_grant_entries(gt);
> +    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < gt_nr_grant_entries; i++ )

This then also calls for a 3rd block_speculation() in nr_grant_entries(),
I think.

Jan



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

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

[Norbert Manthey]

On 2/22/19 16:08, Jan Beulich wrote:
>>>> On 21.02.19 at 09:16, <nmanthey@amazon.de> wrote:
>> @@ -226,10 +228,18 @@ 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:
>> +        /* Make sure we return a value independently of speculative execution */
>> +        block_speculation();
>>          return (grant_entry_header_t*)&shared_entry_v1(t, ref);
>> -    else
>> +    case 2:
>> +        /* Make sure we return a value independently of speculative execution */
>> +        block_speculation();
>>          return &shared_entry_v2(t, ref).hdr;
>> +    }
>> +    return NULL;
>>  }
> I'm not happy with the comment wording, as to me it reads ambiguously
> at best. How about "Make sure the value returned is independent of
> speculative execution"? I'm of course open to even better suggestions,
> in particular by native speakers.
>
> Also please add a blank line
> - between the individual case blocks,
> - before the final (main) return statement.
>
> Plus please add ASSERT_UNREACHABLE() ahead of the NULL return.
I will adapt the function accordingly.
>
>> @@ -963,9 +979,15 @@ map_grant_ref(
>>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>>                   op->ref, rgt->domain->domain_id);
>>  
>> +    /* Make sure the above bound check cannot be bypassed speculatively */
>> +    block_speculation();
>> +
>>      act = active_entry_acquire(rgt, op->ref);
>>      shah = shared_entry_header(rgt, op->ref);
> So shared_entry_header() now has a fence before consuming op->ref.
> Is there anything wrong with swapping these two and dropping the
> extra fence you add above, like you do in acquire_grant_for_copy()? All
> this would seem to require is adding block_speculation() also onto the
> "return NULL" path of shared_entry_header() (where it shouldn't hurt
> at all).
I will swap the two function calls, I missed that in the previous
iteration. I will add the block_speculation() and ASSERT_UNREACHABLE()
at the end of the shared_entry_header function.
>
>> @@ -2946,6 +2981,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>>      grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
>>      int res;
>>      unsigned int i;
>> +    unsigned int gt_nr_grant_entries;
> Rather then lengthening the name by adding a disambiguating prefix,
> how about shortening it to "nr" or "nr_ents" (also elsewhere)? Also
> please add onto the line declaring i instead of adding yet another line
> with the same type.
I will rename the variable to nr_ents and move it to the line above.
>
>> @@ -2969,7 +3005,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++ )
>> +    gt_nr_grant_entries = nr_grant_entries(gt);
>> +    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < gt_nr_grant_entries; i++ )
> This then also calls for a 3rd block_speculation() in nr_grant_entries(),
> I think.

I'll add that one as well.

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