* [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening
@ 2019-09-30 18:24 Andrew Cooper
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN Andrew Cooper
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Andrew Cooper @ 2019-09-30 18:24 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Wei Liu, Andrew Cooper, Norbert Manthey,
Jan Beulich, Roger Pau Monné
The main purpose is patch 2. The "l1tf-barrier" work currently causes a perf
hit and gains no safety, and is therefore unfit for inclusion into Xen 4.13 at
this time.
Andrew Cooper (2):
xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN
xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
docs/misc/xen-command-line.pandoc | 8 +-------
xen/arch/x86/spec_ctrl.c | 17 ++---------------
xen/common/Kconfig | 38 ++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/cpufeatures.h | 2 +-
xen/include/asm-x86/nospec.h | 4 ++--
xen/include/asm-x86/spec_ctrl.h | 1 -
xen/include/xen/nospec.h | 12 ++++++++++++
7 files changed, 56 insertions(+), 26 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN
2019-09-30 18:24 [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening Andrew Cooper
@ 2019-09-30 18:24 ` Andrew Cooper
2019-10-01 10:45 ` Jan Beulich
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it Andrew Cooper
2019-10-01 6:35 ` [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening Jürgen Groß
2 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2019-09-30 18:24 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
There are legitimate circumstance where array hardening is not wanted or
needed. Allow it to be turned off.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
xen/common/Kconfig | 21 +++++++++++++++++++++
xen/include/xen/nospec.h | 12 ++++++++++++
2 files changed, 33 insertions(+)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 16829f6274..9644cc9911 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -77,6 +77,27 @@ config HAS_CHECKPOLICY
string
option env="XEN_HAS_CHECKPOLICY"
+menu "Speculative hardening"
+
+config SPECULATIVE_ARRAY_HARDEN
+ bool "Speculative Array Hardening"
+ default y
+ ---help---
+ Contemporary processors may use speculative execution as a
+ performance optimisation, but this can potentially be abused by an
+ attacker to leak data via speculative sidechannels.
+
+ One source of data leakage is via speculative out-of-bounds array
+ accesses.
+
+ When enabled, specific array accesses which have been deemed liable
+ to be speculatively abused will be hardened to avoid out-of-bounds
+ accesses.
+
+ If unsure, say Y.
+
+endmenu
+
config KEXEC
bool "kexec support"
default y
diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
index 2ac8feccc2..e627a4da52 100644
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -33,6 +33,7 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
}
#endif
+#ifdef CONFIG_SPECULATIVE_ARRAY_HARDEN
/*
* array_index_nospec - sanitize an array index after a bounds check
*
@@ -58,6 +59,17 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
\
(typeof(_i)) (_i & _mask); \
})
+#else
+/* No index hardening. */
+#define array_index_nospec(index, size) \
+({ \
+ typeof(index) _i = (index); \
+ typeof(size) _s = (size); \
+ \
+ (void)_s; \
+ _i; \
+})
+#endif /* CONFIG_SPECULATIVE_ARRAY_HARDEN */
/*
* array_access_nospec - allow nospec access for static size arrays
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-09-30 18:24 [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening Andrew Cooper
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN Andrew Cooper
@ 2019-09-30 18:24 ` Andrew Cooper
2019-09-30 20:17 ` Julien Grall
2019-10-01 12:21 ` Jan Beulich
2019-10-01 6:35 ` [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening Jürgen Groß
2 siblings, 2 replies; 23+ messages in thread
From: Andrew Cooper @ 2019-09-30 18:24 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Wei Liu, Andrew Cooper, Norbert Manthey,
Jan Beulich, Roger Pau Monné
The code generation for barrier_nospec_true() is not correct. We are taking a
perf it from the added fences, but not gaining any speculative safety.
This is caused by inline assembly trying to fight the compiler optimiser, and
the optimiser winning. There is no clear way to achieve safety, so turn the
perf hit off for now.
This also largely reverts 3860d5534df4. The name 'l1tf-barrier', and making
barrier_nospec_true() depend on CONFIG_HVM was constrained by what could be
discussed publicly at the time. Now that MDS is public, neither aspects are
correct.
As l1tf-barrier hasn't been in a release of Xen, and
CONFIG_SPECULATIVE_BRANCH_HARDEN is disabled until we can find a safe way of
implementing the functionality, remove the l1tf-barrier command line option.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Norbert Manthey <nmanthey@amazon.de>
---
docs/misc/xen-command-line.pandoc | 8 +-------
xen/arch/x86/spec_ctrl.c | 17 ++---------------
xen/common/Kconfig | 17 +++++++++++++++++
xen/include/asm-x86/cpufeatures.h | 2 +-
xen/include/asm-x86/nospec.h | 4 ++--
xen/include/asm-x86/spec_ctrl.h | 1 -
6 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index fc64429064..b9c5b822ca 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1932,7 +1932,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,md-clear}=<bool>,
> bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,eager-fpu,
-> l1d-flush,l1tf-barrier}=<bool> ]`
+> l1d-flush}=<bool> ]`
Controls for speculative execution sidechannel mitigations. By default, Xen
will pick the most appropriate mitigations based on compiled in support,
@@ -2004,12 +2004,6 @@ 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
index 4761be81bd..5ea8870981 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -21,7 +21,6 @@
#include <xen/lib.h>
#include <xen/warning.h>
-#include <asm/cpuid.h>
#include <asm/microcode.h>
#include <asm/msr.h>
#include <asm/processor.h>
@@ -53,7 +52,6 @@ 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;
@@ -98,8 +96,6 @@ 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;
@@ -175,8 +171,6 @@ 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;
@@ -337,7 +331,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%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" :
@@ -348,8 +342,7 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
opt_ibpb ? " IBPB" : "",
opt_l1d_flush ? " L1D_FLUSH" : "",
- opt_md_clear_pv || opt_md_clear_hvm ? " VERW" : "",
- opt_l1tf_barrier ? " L1TF_BARRIER" : "");
+ opt_md_clear_pv || opt_md_clear_hvm ? " VERW" : "");
/* L1TF diagnostics, printed if vulnerable or PV shadowing is in use. */
if ( cpu_has_bug_l1tf || opt_pv_l1tf_hwdom || opt_pv_l1tf_domu )
@@ -1034,12 +1027,6 @@ 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/common/Kconfig b/xen/common/Kconfig
index 9644cc9911..d851e63083 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
If unsure, say Y.
+config SPECULATIVE_BRANCH_HARDEN
+ bool "Speculative Branch Hardening"
+ depends on BROKEN
+ ---help---
+ Contemporary processors may use speculative execution as a
+ performance optimisation, but this can potentially be abused by an
+ attacker to leak data via speculative sidechannels.
+
+ One source of misbehaviour is by executing the wrong basic block
+ following a conditional jump.
+
+ When enabled, specific conditions which have been deemed liable to
+ be speculatively abused will be hardened to avoid entering the wrong
+ basic block.
+
+ !!! WARNING - This is broken and doesn't generate safe code !!!
+
endmenu
config KEXEC
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index 91eccf5161..ecb651c35d 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -27,7 +27,7 @@ XEN_CPUFEATURE(XEN_SMAP, X86_SYNTH(11)) /* SMAP gets used by Xen itself
XEN_CPUFEATURE(LFENCE_DISPATCH, X86_SYNTH(12)) /* lfence set as Dispatch Serialising */
XEN_CPUFEATURE(IND_THUNK_LFENCE, X86_SYNTH(13)) /* Use IND_THUNK_LFENCE */
XEN_CPUFEATURE(IND_THUNK_JMP, X86_SYNTH(14)) /* Use IND_THUNK_JMP */
-XEN_CPUFEATURE(SC_L1TF_VULN, X86_SYNTH(15)) /* L1TF protection required */
+/* 15 unused. */
XEN_CPUFEATURE(SC_MSR_PV, X86_SYNTH(16)) /* MSR_SPEC_CTRL used by Xen for PV */
XEN_CPUFEATURE(SC_MSR_HVM, X86_SYNTH(17)) /* MSR_SPEC_CTRL used by Xen for HVM */
XEN_CPUFEATURE(SC_RSB_PV, X86_SYNTH(18)) /* RSB overwrite needed for PV */
diff --git a/xen/include/asm-x86/nospec.h b/xen/include/asm-x86/nospec.h
index 2aa47b3455..df7f9b13d7 100644
--- a/xen/include/asm-x86/nospec.h
+++ b/xen/include/asm-x86/nospec.h
@@ -9,8 +9,8 @@
/* 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);
+#ifdef CONFIG_SPECULATIVE_BRANCH_HARDEN
+ alternative("", "lfence", X86_FEATURE_ALWAYS);
#endif
return true;
}
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 1339ddd7ef..ba03bb42e5 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -37,7 +37,6 @@ 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.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it Andrew Cooper
@ 2019-09-30 20:17 ` Julien Grall
2019-09-30 23:21 ` Andrew Cooper
2019-10-01 12:21 ` Jan Beulich
1 sibling, 1 reply; 23+ messages in thread
From: Julien Grall @ 2019-09-30 20:17 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Juergen Gross, Jan Beulich, Norbert Manthey, Wei Liu,
Roger Pau Monné
Hi,
On 9/30/19 7:24 PM, Andrew Cooper wrote:
> The code generation for barrier_nospec_true() is not correct. We are taking a
> perf it from the added fences, but not gaining any speculative safety.
s/it/hit/?
>
> This is caused by inline assembly trying to fight the compiler optimiser, and
> the optimiser winning. There is no clear way to achieve safety, so turn the
> perf hit off for now.
>
> This also largely reverts 3860d5534df4. The name 'l1tf-barrier', and making
> barrier_nospec_true() depend on CONFIG_HVM was constrained by what could be
> discussed publicly at the time. Now that MDS is public, neither aspects are
> correct.
>
> As l1tf-barrier hasn't been in a release of Xen, and
> CONFIG_SPECULATIVE_BRANCH_HARDEN is disabled until we can find a safe way of
> implementing the functionality, remove the l1tf-barrier command line option.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Norbert Manthey <nmanthey@amazon.de>
> ---
> docs/misc/xen-command-line.pandoc | 8 +-------
> xen/arch/x86/spec_ctrl.c | 17 ++---------------
> xen/common/Kconfig | 17 +++++++++++++++++
I think this wanted to have "THE REST" CCed.
> xen/include/asm-x86/cpufeatures.h | 2 +-
> xen/include/asm-x86/nospec.h | 4 ++--
> xen/include/asm-x86/spec_ctrl.h | 1 -
> 6 files changed, 23 insertions(+), 26 deletions(-)
[...]
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 9644cc9911..d851e63083 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>
> If unsure, say Y.
>
> +config SPECULATIVE_BRANCH_HARDEN
> + bool "Speculative Branch Hardening"
> + depends on BROKEN
> + ---help---
> + Contemporary processors may use speculative execution as a
> + performance optimisation, but this can potentially be abused by an
> + attacker to leak data via speculative sidechannels.
> +
> + One source of misbehaviour is by executing the wrong basic block
> + following a conditional jump.
> +
> + When enabled, specific conditions which have been deemed liable to
> + be speculatively abused will be hardened to avoid entering the wrong
> + basic block.
> +
> + !!! WARNING - This is broken and doesn't generate safe code !!!
Any reason to add that in common code when this is x86 only? My worry is
this gate config gate nothing on Arm so the user may have a false sense
that it can be used (even though it is clearly BROKEN).
Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm and
may confuse user. Although, I don't have a better name so far :/
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-09-30 20:17 ` Julien Grall
@ 2019-09-30 23:21 ` Andrew Cooper
2019-10-01 9:17 ` Julien Grall
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2019-09-30 23:21 UTC (permalink / raw)
To: Julien Grall, Xen-devel
Cc: Juergen Gross, Jan Beulich, Norbert Manthey, Wei Liu,
Roger Pau Monné
On 30/09/2019 21:17, Julien Grall wrote:
> Hi,
>
> On 9/30/19 7:24 PM, Andrew Cooper wrote:
>> The code generation for barrier_nospec_true() is not correct. We are
>> taking a
>> perf it from the added fences, but not gaining any speculative safety.
>
> s/it/hit/?
Yes.
>
>>
>> This is caused by inline assembly trying to fight the compiler
>> optimiser, and
>> the optimiser winning. There is no clear way to achieve safety, so
>> turn the
>> perf hit off for now.
>>
>> This also largely reverts 3860d5534df4. The name 'l1tf-barrier', and
>> making
>> barrier_nospec_true() depend on CONFIG_HVM was constrained by what
>> could be
>> discussed publicly at the time. Now that MDS is public, neither
>> aspects are
>> correct.
>>
>> As l1tf-barrier hasn't been in a release of Xen, and
>> CONFIG_SPECULATIVE_BRANCH_HARDEN is disabled until we can find a safe
>> way of
>> implementing the functionality, remove the l1tf-barrier command line
>> option.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Juergen Gross <jgross@suse.com>
>> CC: Norbert Manthey <nmanthey@amazon.de>
>> ---
>> docs/misc/xen-command-line.pandoc | 8 +-------
>> xen/arch/x86/spec_ctrl.c | 17 ++---------------
>> xen/common/Kconfig | 17 +++++++++++++++++
>
> I think this wanted to have "THE REST" CCed.
>
>> xen/include/asm-x86/cpufeatures.h | 2 +-
>> xen/include/asm-x86/nospec.h | 4 ++--
>> xen/include/asm-x86/spec_ctrl.h | 1 -
>> 6 files changed, 23 insertions(+), 26 deletions(-)
>
> [...]
>
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index 9644cc9911..d851e63083 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>> If unsure, say Y.
>> +config SPECULATIVE_BRANCH_HARDEN
>> + bool "Speculative Branch Hardening"
>> + depends on BROKEN
>> + ---help---
>> + Contemporary processors may use speculative execution as a
>> + performance optimisation, but this can potentially be abused
>> by an
>> + attacker to leak data via speculative sidechannels.
>> +
>> + One source of misbehaviour is by executing the wrong basic block
>> + following a conditional jump.
>> +
>> + When enabled, specific conditions which have been deemed
>> liable to
>> + be speculatively abused will be hardened to avoid entering the
>> wrong
>> + basic block.
>> +
>> + !!! WARNING - This is broken and doesn't generate safe code !!!
>
> Any reason to add that in common code when this is x86 only?
In principle, its not x86 specific.
> My worry is this gate config gate nothing on Arm so the user may have
> a false sense that it can be used (even though it is clearly BROKEN).
>
> Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm and
> may confuse user. Although, I don't have a better name so far :/
The "depends on BROKEN" means it will never show up to a user in
menuconfig, which is why it was only CC to x86, and not to rest.
As for naming, I'm open to suggestions, but this was the best I could
come up with.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening
2019-09-30 18:24 [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening Andrew Cooper
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN Andrew Cooper
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it Andrew Cooper
@ 2019-10-01 6:35 ` Jürgen Groß
2 siblings, 0 replies; 23+ messages in thread
From: Jürgen Groß @ 2019-10-01 6:35 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Wei Liu, Norbert Manthey, Jan Beulich, Roger Pau Monné
On 30.09.19 20:24, Andrew Cooper wrote:
> The main purpose is patch 2. The "l1tf-barrier" work currently causes a perf
> hit and gains no safety, and is therefore unfit for inclusion into Xen 4.13 at
> this time.
>
> Andrew Cooper (2):
> xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN
> xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
>
> docs/misc/xen-command-line.pandoc | 8 +-------
> xen/arch/x86/spec_ctrl.c | 17 ++---------------
> xen/common/Kconfig | 38 ++++++++++++++++++++++++++++++++++++++
> xen/include/asm-x86/cpufeatures.h | 2 +-
> xen/include/asm-x86/nospec.h | 4 ++--
> xen/include/asm-x86/spec_ctrl.h | 1 -
> xen/include/xen/nospec.h | 12 ++++++++++++
> 7 files changed, 56 insertions(+), 26 deletions(-)
>
For the series:
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-09-30 23:21 ` Andrew Cooper
@ 2019-10-01 9:17 ` Julien Grall
2019-10-01 9:22 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2019-10-01 9:17 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Juergen Gross, Jan Beulich, Norbert Manthey, Wei Liu,
Roger Pau Monné
Hi Andrew,
On 01/10/2019 00:21, Andrew Cooper wrote:
> On 30/09/2019 21:17, Julien Grall wrote:
>> Hi,
>>
>> On 9/30/19 7:24 PM, Andrew Cooper wrote:
>>> The code generation for barrier_nospec_true() is not correct. We are
>>> taking a
>>> perf it from the added fences, but not gaining any speculative safety.
>>
>> s/it/hit/?
>
> Yes.
>
>>
>>>
>>> This is caused by inline assembly trying to fight the compiler
>>> optimiser, and
>>> the optimiser winning. There is no clear way to achieve safety, so
>>> turn the
>>> perf hit off for now.
>>>
>>> This also largely reverts 3860d5534df4. The name 'l1tf-barrier', and
>>> making
>>> barrier_nospec_true() depend on CONFIG_HVM was constrained by what
>>> could be
>>> discussed publicly at the time. Now that MDS is public, neither
>>> aspects are
>>> correct.
>>>
>>> As l1tf-barrier hasn't been in a release of Xen, and
>>> CONFIG_SPECULATIVE_BRANCH_HARDEN is disabled until we can find a safe
>>> way of
>>> implementing the functionality, remove the l1tf-barrier command line
>>> option.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Juergen Gross <jgross@suse.com>
>>> CC: Norbert Manthey <nmanthey@amazon.de>
>>> ---
>>> docs/misc/xen-command-line.pandoc | 8 +-------
>>> xen/arch/x86/spec_ctrl.c | 17 ++---------------
>>> xen/common/Kconfig | 17 +++++++++++++++++
>>
>> I think this wanted to have "THE REST" CCed.
>>
>>> xen/include/asm-x86/cpufeatures.h | 2 +-
>>> xen/include/asm-x86/nospec.h | 4 ++--
>>> xen/include/asm-x86/spec_ctrl.h | 1 -
>>> 6 files changed, 23 insertions(+), 26 deletions(-)
>>
>> [...]
>>
>>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>>> index 9644cc9911..d851e63083 100644
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>>> If unsure, say Y.
>>> +config SPECULATIVE_BRANCH_HARDEN
>>> + bool "Speculative Branch Hardening"
>>> + depends on BROKEN
>>> + ---help---
>>> + Contemporary processors may use speculative execution as a
>>> + performance optimisation, but this can potentially be abused
>>> by an
>>> + attacker to leak data via speculative sidechannels.
>>> +
>>> + One source of misbehaviour is by executing the wrong basic block
>>> + following a conditional jump.
>>> +
>>> + When enabled, specific conditions which have been deemed
>>> liable to
>>> + be speculatively abused will be hardened to avoid entering the
>>> wrong
>>> + basic block.
>>> +
>>> + !!! WARNING - This is broken and doesn't generate safe code !!!
>>
>> Any reason to add that in common code when this is x86 only?
>
> In principle, its not x86 specific.
>
>> My worry is this gate config gate nothing on Arm so the user may have
>> a false sense that it can be used (even though it is clearly BROKEN).
>>
>> Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm and
>> may confuse user. Although, I don't have a better name so far :/
>
> The "depends on BROKEN" means it will never show up to a user in
> menuconfig, which is why it was only CC to x86, and not to rest.
What's the long term plan for this option? Are you planning to remove it
completely or just the dependencies on BROKEN?
My concern is if this option will ever become selectable then it will not doing
what's expected on Arm.
So, even if in principle it is arch agnostic, it feels to me this option should
better be implemented in x86/Kconfig.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 9:17 ` Julien Grall
@ 2019-10-01 9:22 ` Jan Beulich
2019-10-01 9:26 ` Julien Grall
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-10-01 9:22 UTC (permalink / raw)
To: Julien Grall
Cc: Juergen Gross, Wei Liu, Andrew Cooper, Norbert Manthey,
Xen-devel, Roger Pau Monné
On 01.10.2019 11:17, Julien Grall wrote:
> On 01/10/2019 00:21, Andrew Cooper wrote:
>> On 30/09/2019 21:17, Julien Grall wrote:
>>> My worry is this gate config gate nothing on Arm so the user may have
>>> a false sense that it can be used (even though it is clearly BROKEN).
>>>
>>> Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm and
>>> may confuse user. Although, I don't have a better name so far :/
>>
>> The "depends on BROKEN" means it will never show up to a user in
>> menuconfig, which is why it was only CC to x86, and not to rest.
>
> What's the long term plan for this option? Are you planning to remove it
> completely or just the dependencies on BROKEN?
>
> My concern is if this option will ever become selectable then it will not doing
> what's expected on Arm.
>
> So, even if in principle it is arch agnostic, it feels to me this option should
> better be implemented in x86/Kconfig.
I don't think so, no. When BROKEN is to be removed, it ought to be
replaced by a suitable "depends on HAVE_*", which Arm could choose
to not select.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 9:22 ` Jan Beulich
@ 2019-10-01 9:26 ` Julien Grall
2019-10-01 9:41 ` Andrew Cooper
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2019-10-01 9:26 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Wei Liu, Andrew Cooper, Norbert Manthey,
Xen-devel, Roger Pau Monné
Hi Jan,
On 01/10/2019 10:22, Jan Beulich wrote:
> On 01.10.2019 11:17, Julien Grall wrote:
>> On 01/10/2019 00:21, Andrew Cooper wrote:
>>> On 30/09/2019 21:17, Julien Grall wrote:
>>>> My worry is this gate config gate nothing on Arm so the user may have
>>>> a false sense that it can be used (even though it is clearly BROKEN).
>>>>
>>>> Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm and
>>>> may confuse user. Although, I don't have a better name so far :/
>>>
>>> The "depends on BROKEN" means it will never show up to a user in
>>> menuconfig, which is why it was only CC to x86, and not to rest.
>>
>> What's the long term plan for this option? Are you planning to remove it
>> completely or just the dependencies on BROKEN?
>>
>> My concern is if this option will ever become selectable then it will not doing
>> what's expected on Arm.
>>
>> So, even if in principle it is arch agnostic, it feels to me this option should
>> better be implemented in x86/Kconfig.
>
> I don't think so, no. When BROKEN is to be removed, it ought to be
> replaced by a suitable "depends on HAVE_*", which Arm could choose
> to not select.
So there are an expectation this option will be used by common code in the future?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 9:26 ` Julien Grall
@ 2019-10-01 9:41 ` Andrew Cooper
2019-10-01 9:45 ` Julien Grall
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2019-10-01 9:41 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 01/10/2019 10:26, Julien Grall wrote:
> Hi Jan,
>
> On 01/10/2019 10:22, Jan Beulich wrote:
>> On 01.10.2019 11:17, Julien Grall wrote:
>>> On 01/10/2019 00:21, Andrew Cooper wrote:
>>>> On 30/09/2019 21:17, Julien Grall wrote:
>>>>> My worry is this gate config gate nothing on Arm so the user may have
>>>>> a false sense that it can be used (even though it is clearly BROKEN).
>>>>>
>>>>> Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm
>>>>> and
>>>>> may confuse user. Although, I don't have a better name so far :/
>>>>
>>>> The "depends on BROKEN" means it will never show up to a user in
>>>> menuconfig, which is why it was only CC to x86, and not to rest.
>>>
>>> What's the long term plan for this option? Are you planning to
>>> remove it
>>> completely or just the dependencies on BROKEN?
>>>
>>> My concern is if this option will ever become selectable then it
>>> will not doing
>>> what's expected on Arm.
>>>
>>> So, even if in principle it is arch agnostic, it feels to me this
>>> option should
>>> better be implemented in x86/Kconfig.
>>
>> I don't think so, no. When BROKEN is to be removed, it ought to be
>> replaced by a suitable "depends on HAVE_*", which Arm could choose
>> to not select.
>
> So there are an expectation this option will be used by common code in
> the future?
It already is. ARM has stubs for evaluate_nospec() etc.
My gut feeling is that the only way this is going to be resolved sanely
is with a compiler feature or plugin, at which point it reasonably can
be cross-arch.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 9:41 ` Andrew Cooper
@ 2019-10-01 9:45 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2019-10-01 9:45 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
Hi Andrew,
On 01/10/2019 10:41, Andrew Cooper wrote:
> On 01/10/2019 10:26, Julien Grall wrote:
>> Hi Jan,
>>
>> On 01/10/2019 10:22, Jan Beulich wrote:
>>> On 01.10.2019 11:17, Julien Grall wrote:
>>>> On 01/10/2019 00:21, Andrew Cooper wrote:
>>>>> On 30/09/2019 21:17, Julien Grall wrote:
>>>>>> My worry is this gate config gate nothing on Arm so the user may have
>>>>>> a false sense that it can be used (even though it is clearly BROKEN).
>>>>>>
>>>>>> Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm
>>>>>> and
>>>>>> may confuse user. Although, I don't have a better name so far :/
>>>>>
>>>>> The "depends on BROKEN" means it will never show up to a user in
>>>>> menuconfig, which is why it was only CC to x86, and not to rest.
>>>>
>>>> What's the long term plan for this option? Are you planning to
>>>> remove it
>>>> completely or just the dependencies on BROKEN?
>>>>
>>>> My concern is if this option will ever become selectable then it
>>>> will not doing
>>>> what's expected on Arm.
>>>>
>>>> So, even if in principle it is arch agnostic, it feels to me this
>>>> option should
>>>> better be implemented in x86/Kconfig.
>>>
>>> I don't think so, no. When BROKEN is to be removed, it ought to be
>>> replaced by a suitable "depends on HAVE_*", which Arm could choose
>>> to not select.
>>
>> So there are an expectation this option will be used by common code in
>> the future?
>
> It already is. ARM has stubs for evaluate_nospec() etc.
>
> My gut feeling is that the only way this is going to be resolved sanely
> is with a compiler feature or plugin, at which point it reasonably can
> be cross-arch.
Fair enough. I don't have any more concern then. I will have a think about the
naming or the possibly to merge the two configs (the common and arm ones) together.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN Andrew Cooper
@ 2019-10-01 10:45 ` Jan Beulich
2019-10-01 12:30 ` Andrew Cooper
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-10-01 10:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monné
On 30.09.2019 20:24, Andrew Cooper wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -77,6 +77,27 @@ config HAS_CHECKPOLICY
> string
> option env="XEN_HAS_CHECKPOLICY"
>
> +menu "Speculative hardening"
> +
> +config SPECULATIVE_ARRAY_HARDEN
Seeing also the new item in patch 2 - wouldn't it be better for them all
to have (just) a common prefix, rather than common prefix and a common
suffix. E.g. SPECULATIVE_HARDEN_ARRAYS here and SPECULATIVE_HARDEN_BRANCHES
there?
> --- a/xen/include/xen/nospec.h
> +++ b/xen/include/xen/nospec.h
> @@ -33,6 +33,7 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
> }
> #endif
>
> +#ifdef CONFIG_SPECULATIVE_ARRAY_HARDEN
> /*
> * array_index_nospec - sanitize an array index after a bounds check
> *
> @@ -58,6 +59,17 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
> \
> (typeof(_i)) (_i & _mask); \
> })
> +#else
> +/* No index hardening. */
> +#define array_index_nospec(index, size) \
> +({ \
> + typeof(index) _i = (index); \
> + typeof(size) _s = (size); \
> + \
> + (void)_s; \
> + _i; \
> +})
Why not the simpler
#define array_index_nospec(index, size) \
({ \
(void)(size); \
(index); \
})
at which point it would seem feasible to avoid the use of compiler
extensions altogether by making it
#define array_index_nospec(index, size) ((void)(size), (index))
?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it Andrew Cooper
2019-09-30 20:17 ` Julien Grall
@ 2019-10-01 12:21 ` Jan Beulich
2019-10-01 12:51 ` Andrew Cooper
1 sibling, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-10-01 12:21 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 30.09.2019 20:24, Andrew Cooper wrote:
> The code generation for barrier_nospec_true() is not correct. We are taking a
> perf it from the added fences, but not gaining any speculative safety.
You want to be more specific here, I think: ISTR you saying that the LFENCEs
get inserted at the wrong place. IIRC we want them on either side of a
conditional branch, i.e. immediately following a branch insn as well as right
at the branch target. I've taken, as a simple example,
p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
code (in a non-debug build). Hence either I'm mis-remembering what we want
things to look like, or there's more to it than code generation simply being
"not correct".
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>
> If unsure, say Y.
>
> +config SPECULATIVE_BRANCH_HARDEN
> + bool "Speculative Branch Hardening"
> + depends on BROKEN
> + ---help---
> + Contemporary processors may use speculative execution as a
> + performance optimisation, but this can potentially be abused by an
> + attacker to leak data via speculative sidechannels.
> +
> + One source of misbehaviour is by executing the wrong basic block
> + following a conditional jump.
> +
> + When enabled, specific conditions which have been deemed liable to
> + be speculatively abused will be hardened to avoid entering the wrong
> + basic block.
> +
> + !!! WARNING - This is broken and doesn't generate safe code !!!
Not being a native speaker, this read to me as "is broken in general",
whereas the brokenness is that according to your analysis safe code
does not result. Therefore how about "This is broken in that it doesn't
generate safe code"?
> --- a/xen/include/asm-x86/nospec.h
> +++ b/xen/include/asm-x86/nospec.h
> @@ -9,8 +9,8 @@
> /* 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);
> +#ifdef CONFIG_SPECULATIVE_BRANCH_HARDEN
> + alternative("", "lfence", X86_FEATURE_ALWAYS);
Why alternative() then and not just asm()?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN
2019-10-01 10:45 ` Jan Beulich
@ 2019-10-01 12:30 ` Andrew Cooper
0 siblings, 0 replies; 23+ messages in thread
From: Andrew Cooper @ 2019-10-01 12:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: Juergen Gross, Xen-devel, Wei Liu, Roger Pau Monné
On 01/10/2019 11:45, Jan Beulich wrote:
> On 30.09.2019 20:24, Andrew Cooper wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -77,6 +77,27 @@ config HAS_CHECKPOLICY
>> string
>> option env="XEN_HAS_CHECKPOLICY"
>>
>> +menu "Speculative hardening"
>> +
>> +config SPECULATIVE_ARRAY_HARDEN
> Seeing also the new item in patch 2 - wouldn't it be better for them all
> to have (just) a common prefix, rather than common prefix and a common
> suffix. E.g. SPECULATIVE_HARDEN_ARRAYS here and SPECULATIVE_HARDEN_BRANCHES
> there?
Can do.
>
>> --- a/xen/include/xen/nospec.h
>> +++ b/xen/include/xen/nospec.h
>> @@ -33,6 +33,7 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>> }
>> #endif
>>
>> +#ifdef CONFIG_SPECULATIVE_ARRAY_HARDEN
>> /*
>> * array_index_nospec - sanitize an array index after a bounds check
>> *
>> @@ -58,6 +59,17 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>> \
>> (typeof(_i)) (_i & _mask); \
>> })
>> +#else
>> +/* No index hardening. */
>> +#define array_index_nospec(index, size) \
>> +({ \
>> + typeof(index) _i = (index); \
>> + typeof(size) _s = (size); \
>> + \
>> + (void)_s; \
>> + _i; \
>> +})
> Why not the simpler
>
> #define array_index_nospec(index, size) \
> ({ \
> (void)(size); \
> (index); \
> })
>
> at which point it would seem feasible to avoid the use of compiler
> extensions altogether by making it
>
> #define array_index_nospec(index, size) ((void)(size), (index))
Huh - I tried that first, and GCC was distinctly unhappy. It turns out
to be the bracketing of size, which when omitted, causes:
/local/xen.git/xen/include/xen/nospec.h:66:42: error: void value not
ignored as it ought to be
#define array_index_nospec(index, size) ((void)size, (index))
argo.c:2174:16: note: in expansion of macro ‘array_index_nospec’
niov = array_index_nospec(arg3, XEN_ARGO_MAXIOV + 1);
^~~~~~~~~~~~~~~~~~
I'll switch to this version.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 12:21 ` Jan Beulich
@ 2019-10-01 12:51 ` Andrew Cooper
2019-10-01 14:32 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2019-10-01 12:51 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 01/10/2019 13:21, Jan Beulich wrote:
> On 30.09.2019 20:24, Andrew Cooper wrote:
>> The code generation for barrier_nospec_true() is not correct. We are taking a
>> perf it from the added fences, but not gaining any speculative safety.
> You want to be more specific here, I think: ISTR you saying that the LFENCEs
> get inserted at the wrong place.
Correct.
> IIRC we want them on either side of a
> conditional branch, i.e. immediately following a branch insn as well as right
> at the branch target.
Specifically, they must be at the head of both basic blocks following
the conditional jump.
> I've taken, as a simple example,
> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
> code (in a non-debug build). Hence either I'm mis-remembering what we want
> things to look like, or there's more to it than code generation simply being
> "not correct".
This example demonstrates the problem, and actually throws a further
spanner in the works of how make this safe, which hadn't occurred to me
before.
The instruction stream from a caller of p2m_mem_access_sanity_check()
will look something like this:
call p2m_mem_access_sanity_check
...
lfence
...
ret
cmp $0, %eax
jne ...
Which is unsafe, because the only safe way to arrange this code would be:
call p2m_mem_access_sanity_check
...
ret
cmp $0, %eax
jne 1f
lfence
...
1: lfence
...
There is absolutely no possible way for inline assembly to be used to
propagate this safety property across translation units. This is going
to have to be an attribute of some form or another handled by the compiler.
>
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>>
>> If unsure, say Y.
>>
>> +config SPECULATIVE_BRANCH_HARDEN
>> + bool "Speculative Branch Hardening"
>> + depends on BROKEN
>> + ---help---
>> + Contemporary processors may use speculative execution as a
>> + performance optimisation, but this can potentially be abused by an
>> + attacker to leak data via speculative sidechannels.
>> +
>> + One source of misbehaviour is by executing the wrong basic block
>> + following a conditional jump.
>> +
>> + When enabled, specific conditions which have been deemed liable to
>> + be speculatively abused will be hardened to avoid entering the wrong
>> + basic block.
>> +
>> + !!! WARNING - This is broken and doesn't generate safe code !!!
> Not being a native speaker, this read to me as "is broken in general",
> whereas the brokenness is that according to your analysis safe code
> does not result. Therefore how about "This is broken in that it doesn't
> generate safe code"?
I wouldn't necessarily agree with the "in general" implication, but
given the lack of clarity, a better option would be:
!!! WARNING - This option doesn't work as intended. It does not generate
speculatively safe code !!!
>
>> --- a/xen/include/asm-x86/nospec.h
>> +++ b/xen/include/asm-x86/nospec.h
>> @@ -9,8 +9,8 @@
>> /* 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);
>> +#ifdef CONFIG_SPECULATIVE_BRANCH_HARDEN
>> + alternative("", "lfence", X86_FEATURE_ALWAYS);
> Why alternative() then and not just asm()?
Ok.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 12:51 ` Andrew Cooper
@ 2019-10-01 14:32 ` Jan Beulich
2019-10-01 15:37 ` Andrew Cooper
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-10-01 14:32 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 01.10.2019 14:51, Andrew Cooper wrote:
> On 01/10/2019 13:21, Jan Beulich wrote:
>> On 30.09.2019 20:24, Andrew Cooper wrote:
>>> The code generation for barrier_nospec_true() is not correct. We are taking a
>>> perf it from the added fences, but not gaining any speculative safety.
>> You want to be more specific here, I think: ISTR you saying that the LFENCEs
>> get inserted at the wrong place.
>
> Correct.
>
>> IIRC we want them on either side of a
>> conditional branch, i.e. immediately following a branch insn as well as right
>> at the branch target.
>
> Specifically, they must be at the head of both basic blocks following
> the conditional jump.
>
>> I've taken, as a simple example,
>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>> things to look like, or there's more to it than code generation simply being
>> "not correct".
>
> This example demonstrates the problem, and actually throws a further
> spanner in the works of how make this safe, which hadn't occurred to me
> before.
>
> The instruction stream from a caller of p2m_mem_access_sanity_check()
> will look something like this:
>
> call p2m_mem_access_sanity_check
> ...
> lfence
> ...
> ret
> cmp $0, %eax
> jne ...
>
> Which is unsafe, because the only safe way to arrange this code would be:
>
> call p2m_mem_access_sanity_check
> ...
> ret
> cmp $0, %eax
> jne 1f
> lfence
> ...
> 1: lfence
> ...
>
> There is absolutely no possible way for inline assembly to be used to
> propagate this safety property across translation units. This is going
> to have to be an attribute of some form or another handled by the compiler.
But you realize that this particular example is basically a more
complex is_XYZ() check, which could be dealt with by inlining the
function. Of course there are going to be larger functions where
the result wants to be guarded like you say. But just like the
addition of the nospec macros to various is_XYZ() functions is a
manual operation (as long the compiler doesn't help), it would in
that case be a matter of latching the return value into a local
variable and using an appropriate guarding construct when
evaluating it.
So I'm afraid for now I still can't agree with your "not correct"
assessment - the generated code in the example looks correct to
me, and if further guarding was needed in users of this particular
function, it would be those users which would need further
massaging.
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>>>
>>> If unsure, say Y.
>>>
>>> +config SPECULATIVE_BRANCH_HARDEN
>>> + bool "Speculative Branch Hardening"
>>> + depends on BROKEN
>>> + ---help---
>>> + Contemporary processors may use speculative execution as a
>>> + performance optimisation, but this can potentially be abused by an
>>> + attacker to leak data via speculative sidechannels.
>>> +
>>> + One source of misbehaviour is by executing the wrong basic block
>>> + following a conditional jump.
>>> +
>>> + When enabled, specific conditions which have been deemed liable to
>>> + be speculatively abused will be hardened to avoid entering the wrong
>>> + basic block.
>>> +
>>> + !!! WARNING - This is broken and doesn't generate safe code !!!
>> Not being a native speaker, this read to me as "is broken in general",
>> whereas the brokenness is that according to your analysis safe code
>> does not result. Therefore how about "This is broken in that it doesn't
>> generate safe code"?
>
> I wouldn't necessarily agree with the "in general" implication, but
> given the lack of clarity, a better option would be:
>
> !!! WARNING - This option doesn't work as intended. It does not generate
> speculatively safe code !!!
Fine with me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 14:32 ` Jan Beulich
@ 2019-10-01 15:37 ` Andrew Cooper
2019-10-02 8:24 ` Jan Beulich
2019-10-02 9:50 ` Wieczorkiewicz, Pawel
0 siblings, 2 replies; 23+ messages in thread
From: Andrew Cooper @ 2019-10-01 15:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 01/10/2019 15:32, Jan Beulich wrote:
> On 01.10.2019 14:51, Andrew Cooper wrote:
>> On 01/10/2019 13:21, Jan Beulich wrote:
>>> On 30.09.2019 20:24, Andrew Cooper wrote:
>>>> The code generation for barrier_nospec_true() is not correct. We are taking a
>>>> perf it from the added fences, but not gaining any speculative safety.
>>> You want to be more specific here, I think: ISTR you saying that the LFENCEs
>>> get inserted at the wrong place.
>> Correct.
>>
>>> IIRC we want them on either side of a
>>> conditional branch, i.e. immediately following a branch insn as well as right
>>> at the branch target.
>> Specifically, they must be at the head of both basic blocks following
>> the conditional jump.
>>
>>> I've taken, as a simple example,
>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>>> things to look like, or there's more to it than code generation simply being
>>> "not correct".
>> This example demonstrates the problem, and actually throws a further
>> spanner in the works of how make this safe, which hadn't occurred to me
>> before.
>>
>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>> will look something like this:
>>
>> call p2m_mem_access_sanity_check
>> ...
>> lfence
>> ...
>> ret
>> cmp $0, %eax
>> jne ...
>>
>> Which is unsafe, because the only safe way to arrange this code would be:
>>
>> call p2m_mem_access_sanity_check
>> ...
>> ret
>> cmp $0, %eax
>> jne 1f
>> lfence
>> ...
>> 1: lfence
>> ...
>>
>> There is absolutely no possible way for inline assembly to be used to
>> propagate this safety property across translation units. This is going
>> to have to be an attribute of some form or another handled by the compiler.
> But you realize that this particular example is basically a more
> complex is_XYZ() check, which could be dealt with by inlining the
> function. Of course there are going to be larger functions where
> the result wants to be guarded like you say. But just like the
> addition of the nospec macros to various is_XYZ() functions is a
> manual operation (as long the compiler doesn't help), it would in
> that case be a matter of latching the return value into a local
> variable and using an appropriate guarding construct when
> evaluating it.
And this reasoning demonstrates yet another problem (this one was raised
at the meeting in Chicago).
evaluate_nospec() is not a useful construct if it needs inserting at
every higher level predicate to result in safe code. This is
boarderline-impossible to review for, and extremely easy to break
accidentally.
>
> So I'm afraid for now I still can't agree with your "not correct"
> assessment - the generated code in the example looks correct to
> me, and if further guarding was needed in users of this particular
> function, it would be those users which would need further
> massaging.
Safety against spectre v1 is not a matter of opinion.
To protect against speculatively executing the wrong basic block, the
pipeline must execute the conditional jump first, *then* hit an lfence
to serialise the instruction stream and revector in the case of
incorrect speculation.
The other way around is not safe. Serialising the instruction stream
doesn't do anything to protect against the attacker taking control of a
later branch.
The bigger problem is to do with classifying what we are protecting
against. In the case of is_control_domain(), it is any action based on
the result of the decision. For is_{pv,hvm}_domain(), is only (to a
first approximation) speculative type confusion into the pv/hvm unions
(which in practice extends to calling pv_/hvm_ functions as well).
As for the real concrete breakages. In a staging build with GCC 6
$ objdump -d xen-syms | grep '<is_hvm_domain>:' | wc -l
18
$ objdump -d xen-syms | grep '<is_pv_domain>:' | wc -l
9
All of which have the lfence too early to protect against speculative
type confusion.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 15:37 ` Andrew Cooper
@ 2019-10-02 8:24 ` Jan Beulich
2019-10-02 19:31 ` Andrew Cooper
2019-10-02 9:50 ` Wieczorkiewicz, Pawel
1 sibling, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2019-10-02 8:24 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 01.10.2019 17:37, Andrew Cooper wrote:
> On 01/10/2019 15:32, Jan Beulich wrote:
>> On 01.10.2019 14:51, Andrew Cooper wrote:
>>> On 01/10/2019 13:21, Jan Beulich wrote:
>>>> On 30.09.2019 20:24, Andrew Cooper wrote:
>>>>> The code generation for barrier_nospec_true() is not correct. We are taking a
>>>>> perf it from the added fences, but not gaining any speculative safety.
>>>> You want to be more specific here, I think: ISTR you saying that the LFENCEs
>>>> get inserted at the wrong place.
>>> Correct.
>>>
>>>> IIRC we want them on either side of a
>>>> conditional branch, i.e. immediately following a branch insn as well as right
>>>> at the branch target.
>>> Specifically, they must be at the head of both basic blocks following
>>> the conditional jump.
>>>
>>>> I've taken, as a simple example,
>>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>>>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>>>> things to look like, or there's more to it than code generation simply being
>>>> "not correct".
>>> This example demonstrates the problem, and actually throws a further
>>> spanner in the works of how make this safe, which hadn't occurred to me
>>> before.
>>>
>>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>>> will look something like this:
>>>
>>> call p2m_mem_access_sanity_check
>>> ...
>>> lfence
>>> ...
>>> ret
>>> cmp $0, %eax
>>> jne ...
>>>
>>> Which is unsafe, because the only safe way to arrange this code would be:
>>>
>>> call p2m_mem_access_sanity_check
>>> ...
>>> ret
>>> cmp $0, %eax
>>> jne 1f
>>> lfence
>>> ...
>>> 1: lfence
>>> ...
>>>
>>> There is absolutely no possible way for inline assembly to be used to
>>> propagate this safety property across translation units. This is going
>>> to have to be an attribute of some form or another handled by the compiler.
>> But you realize that this particular example is basically a more
>> complex is_XYZ() check, which could be dealt with by inlining the
>> function. Of course there are going to be larger functions where
>> the result wants to be guarded like you say. But just like the
>> addition of the nospec macros to various is_XYZ() functions is a
>> manual operation (as long the compiler doesn't help), it would in
>> that case be a matter of latching the return value into a local
>> variable and using an appropriate guarding construct when
>> evaluating it.
>
> And this reasoning demonstrates yet another problem (this one was raised
> at the meeting in Chicago).
>
> evaluate_nospec() is not a useful construct if it needs inserting at
> every higher level predicate to result in safe code. This is
> boarderline-impossible to review for, and extremely easy to break
> accidentally.
I agree; since evaluate_nospec() insertion need is generally a hard
to investigate / review action, I don#t consider this unexpected.
>> So I'm afraid for now I still can't agree with your "not correct"
>> assessment - the generated code in the example looks correct to
>> me, and if further guarding was needed in users of this particular
>> function, it would be those users which would need further
>> massaging.
>
> Safety against spectre v1 is not a matter of opinion.
>
> To protect against speculatively executing the wrong basic block, the
> pipeline must execute the conditional jump first, *then* hit an lfence
> to serialise the instruction stream and revector in the case of
> incorrect speculation.
>
> The other way around is not safe. Serialising the instruction stream
> doesn't do anything to protect against the attacker taking control of a
> later branch.
>
> The bigger problem is to do with classifying what we are protecting
> against. In the case of is_control_domain(), it is any action based on
> the result of the decision. For is_{pv,hvm}_domain(), is only (to a
> first approximation) speculative type confusion into the pv/hvm unions
> (which in practice extends to calling pv_/hvm_ functions as well).
>
> As for the real concrete breakages. In a staging build with GCC 6
>
> $ objdump -d xen-syms | grep '<is_hvm_domain>:' | wc -l
> 18
> $ objdump -d xen-syms | grep '<is_pv_domain>:' | wc -l
> 9
>
> All of which have the lfence too early to protect against speculative
> type confusion.
And all of which are because, other than I think it was originally
intended, the functions still aren't always_inline.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-01 15:37 ` Andrew Cooper
2019-10-02 8:24 ` Jan Beulich
@ 2019-10-02 9:50 ` Wieczorkiewicz, Pawel
2019-10-02 21:02 ` Andrew Cooper
1 sibling, 1 reply; 23+ messages in thread
From: Wieczorkiewicz, Pawel @ 2019-10-02 9:50 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Wei Liu, Manthey, Norbert, Jan Beulich, Xen-devel,
Roger Pau Monné
> On 1. Oct 2019, at 17:37, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 01/10/2019 15:32, Jan Beulich wrote:
>> On 01.10.2019 14:51, Andrew Cooper wrote:
>>> On 01/10/2019 13:21, Jan Beulich wrote:
>>>> On 30.09.2019 20:24, Andrew Cooper wrote:
>>>>> The code generation for barrier_nospec_true() is not correct. We are taking a
>>>>> perf it from the added fences, but not gaining any speculative safety.
>>>> You want to be more specific here, I think: ISTR you saying that the LFENCEs
>>>> get inserted at the wrong place.
>>> Correct.
>>>
>>>> IIRC we want them on either side of a
>>>> conditional branch, i.e. immediately following a branch insn as well as right
>>>> at the branch target.
>>> Specifically, they must be at the head of both basic blocks following
>>> the conditional jump.
>>>
>>>> I've taken, as a simple example,
>>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>>>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>>>> things to look like, or there's more to it than code generation simply being
>>>> "not correct".
>>> This example demonstrates the problem, and actually throws a further
>>> spanner in the works of how make this safe, which hadn't occurred to me
>>> before.
>>>
>>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>>> will look something like this:
>>>
>>> call p2m_mem_access_sanity_check
>>> ...
>>> lfence
>>> ...
>>> ret
>>> cmp $0, %eax
>>> jne ...
>>>
>>> Which is unsafe, because the only safe way to arrange this code would be:
>>>
>>> call p2m_mem_access_sanity_check
>>> ...
>>> ret
>>> cmp $0, %eax
>>> jne 1f
>>> lfence
>>> ...
>>> 1: lfence
>>> …
>>>
Does it mean the CPU speculates over a direct call (assuming no #PF etc) and
assumes some default return value to be used?
If not, maybe we should be more concerned about the value the cache-loading
gadget speculates with, rather than the sheer speculation over the branch.
Am I mis(understanding) something here? I would be thankful for explanation.
>>> There is absolutely no possible way for inline assembly to be used to
>>> propagate this safety property across translation units. This is going
>>> to have to be an attribute of some form or another handled by the compiler.
>> But you realize that this particular example is basically a more
>> complex is_XYZ() check, which could be dealt with by inlining the
>> function. Of course there are going to be larger functions where
>> the result wants to be guarded like you say. But just like the
>> addition of the nospec macros to various is_XYZ() functions is a
>> manual operation (as long the compiler doesn't help), it would in
>> that case be a matter of latching the return value into a local
>> variable and using an appropriate guarding construct when
>> evaluating it.
>
> And this reasoning demonstrates yet another problem (this one was raised
> at the meeting in Chicago).
>
> evaluate_nospec() is not a useful construct if it needs inserting at
> every higher level predicate to result in safe code. This is
> boarderline-impossible to review for, and extremely easy to break
> accidentally.
>
>>
>> So I'm afraid for now I still can't agree with your "not correct"
>> assessment - the generated code in the example looks correct to
>> me, and if further guarding was needed in users of this particular
>> function, it would be those users which would need further
>> massaging.
>
> Safety against spectre v1 is not a matter of opinion.
>
But the hardening wasn’t about spectre v1, but about cache-load gadgets?
> To protect against speculatively executing the wrong basic block, the
> pipeline must execute the conditional jump first, *then* hit an lfence
> to serialise the instruction stream and revector in the case of
> incorrect speculation.
That’s true, but there are also 2 aspects worth mentioning:
1) Example:
jne 1
PRIVILEGED
1:
ALWAYS_SAFE
We do not necessarily have to cover the 1: path with an lfence?
We could allow speculation there, as it is harmless.
2) cache-load protection
It might be ok to speculate over a conditional branch, when we can
guarantee that the value to be used in a dereference is not out-of-bound.
In that case an lfence is used to latch the value in the register. We can
still speculate over the branch and reach the dereference, but with a sane value.
I agree that lfence might not give us 100% security in every potential case,
but that is what "hardening" gives you...
>
> The other way around is not safe. Serialising the instruction stream
> doesn't do anything to protect against the attacker taking control of a
> later branch.
>
Sure, but it may do something about the value used to dereference memory
when the speculation happens over the branch.
> The bigger problem is to do with classifying what we are protecting
> against. In the case of is_control_domain(), it is any action based on
> the result of the decision. For is_{pv,hvm}_domain(), is only (to a
> first approximation) speculative type confusion into the pv/hvm unions
> (which in practice extends to calling pv_/hvm_ functions as well).
>
> As for the real concrete breakages. In a staging build with GCC 6
>
> $ objdump -d xen-syms | grep '<is_hvm_domain>:' | wc -l
> 18
> $ objdump -d xen-syms | grep '<is_pv_domain>:' | wc -l
> 9
>
> All of which have the lfence too early to protect against speculative
> type confusion.
>
> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-02 8:24 ` Jan Beulich
@ 2019-10-02 19:31 ` Andrew Cooper
2019-10-04 8:55 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2019-10-02 19:31 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 02/10/2019 09:24, Jan Beulich wrote:
> On 01.10.2019 17:37, Andrew Cooper wrote:
>> On 01/10/2019 15:32, Jan Beulich wrote:
>>> On 01.10.2019 14:51, Andrew Cooper wrote:
>>>> On 01/10/2019 13:21, Jan Beulich wrote:
>>>>> On 30.09.2019 20:24, Andrew Cooper wrote:
>>>>>> The code generation for barrier_nospec_true() is not correct. We are taking a
>>>>>> perf it from the added fences, but not gaining any speculative safety.
>>>>> You want to be more specific here, I think: ISTR you saying that the LFENCEs
>>>>> get inserted at the wrong place.
>>>> Correct.
>>>>
>>>>> IIRC we want them on either side of a
>>>>> conditional branch, i.e. immediately following a branch insn as well as right
>>>>> at the branch target.
>>>> Specifically, they must be at the head of both basic blocks following
>>>> the conditional jump.
>>>>
>>>>> I've taken, as a simple example,
>>>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>>>>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>>>>> things to look like, or there's more to it than code generation simply being
>>>>> "not correct".
>>>> This example demonstrates the problem, and actually throws a further
>>>> spanner in the works of how make this safe, which hadn't occurred to me
>>>> before.
>>>>
>>>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>>>> will look something like this:
>>>>
>>>> call p2m_mem_access_sanity_check
>>>> ...
>>>> lfence
>>>> ...
>>>> ret
>>>> cmp $0, %eax
>>>> jne ...
>>>>
>>>> Which is unsafe, because the only safe way to arrange this code would be:
>>>>
>>>> call p2m_mem_access_sanity_check
>>>> ...
>>>> ret
>>>> cmp $0, %eax
>>>> jne 1f
>>>> lfence
>>>> ...
>>>> 1: lfence
>>>> ...
>>>>
>>>> There is absolutely no possible way for inline assembly to be used to
>>>> propagate this safety property across translation units. This is going
>>>> to have to be an attribute of some form or another handled by the compiler.
>>> But you realize that this particular example is basically a more
>>> complex is_XYZ() check, which could be dealt with by inlining the
>>> function. Of course there are going to be larger functions where
>>> the result wants to be guarded like you say. But just like the
>>> addition of the nospec macros to various is_XYZ() functions is a
>>> manual operation (as long the compiler doesn't help), it would in
>>> that case be a matter of latching the return value into a local
>>> variable and using an appropriate guarding construct when
>>> evaluating it.
>> And this reasoning demonstrates yet another problem (this one was raised
>> at the meeting in Chicago).
>>
>> evaluate_nospec() is not a useful construct if it needs inserting at
>> every higher level predicate to result in safe code. This is
>> boarderline-impossible to review for, and extremely easy to break
>> accidentally.
> I agree; since evaluate_nospec() insertion need is generally a hard
> to investigate / review action, I don#t consider this unexpected.
>
>>> So I'm afraid for now I still can't agree with your "not correct"
>>> assessment - the generated code in the example looks correct to
>>> me, and if further guarding was needed in users of this particular
>>> function, it would be those users which would need further
>>> massaging.
>> Safety against spectre v1 is not a matter of opinion.
>>
>> To protect against speculatively executing the wrong basic block, the
>> pipeline must execute the conditional jump first, *then* hit an lfence
>> to serialise the instruction stream and revector in the case of
>> incorrect speculation.
>>
>> The other way around is not safe. Serialising the instruction stream
>> doesn't do anything to protect against the attacker taking control of a
>> later branch.
>>
>> The bigger problem is to do with classifying what we are protecting
>> against. In the case of is_control_domain(), it is any action based on
>> the result of the decision. For is_{pv,hvm}_domain(), is only (to a
>> first approximation) speculative type confusion into the pv/hvm unions
>> (which in practice extends to calling pv_/hvm_ functions as well).
>>
>> As for the real concrete breakages. In a staging build with GCC 6
>>
>> $ objdump -d xen-syms | grep '<is_hvm_domain>:' | wc -l
>> 18
>> $ objdump -d xen-syms | grep '<is_pv_domain>:' | wc -l
>> 9
>>
>> All of which have the lfence too early to protect against speculative
>> type confusion.
> And all of which are because, other than I think it was originally
> intended, the functions still aren't always_inline.
Right, but if we force is_hvm_domain() to be always_inline, then
is_hvm_vcpu() gets out-of-lined.
This turns into a game of whack-a-mole, where any predicate wrapping
something with an embedded evaluate_nospec() breaks the safety.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-02 9:50 ` Wieczorkiewicz, Pawel
@ 2019-10-02 21:02 ` Andrew Cooper
2019-10-04 11:46 ` Wieczorkiewicz, Pawel
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2019-10-02 21:02 UTC (permalink / raw)
To: Wieczorkiewicz, Pawel
Cc: Juergen Gross, Wei Liu, Manthey, Norbert, Jan Beulich, Xen-devel,
Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 7366 bytes --]
On 02/10/2019 10:50, Wieczorkiewicz, Pawel wrote:
>>>>> I've taken, as a simple example,
>>>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>>>>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>>>>> things to look like, or there's more to it than code generation simply being
>>>>> "not correct".
>>>> This example demonstrates the problem, and actually throws a further
>>>> spanner in the works of how make this safe, which hadn't occurred to me
>>>> before.
>>>>
>>>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>>>> will look something like this:
>>>>
>>>> call p2m_mem_access_sanity_check
>>>> ...
>>>> lfence
>>>> ...
>>>> ret
>>>> cmp $0, %eax
>>>> jne ...
>>>>
>>>> Which is unsafe, because the only safe way to arrange this code would be:
>>>>
>>>> call p2m_mem_access_sanity_check
>>>> ...
>>>> ret
>>>> cmp $0, %eax
>>>> jne 1f
>>>> lfence
>>>> ...
>>>> 1: lfence
>>>> …
>>>>
Answering out of order, because I think this will make things clearer.
> But the hardening wasn’t about spectre v1, but about cache-load gadgets?
Ultimately, yes - the goal is cache load gadgets.
Cache load gadgets are any memory read, where the attacker can influence
the position of the load. The easy case to think about is the first
half of a Spectre v1 gadget (i.e. the first memory load), but a second
common case is a simple memory read with the wrong base pointer (as
demonstrated clearly by SpectreGS and CVE-2019-1125).
A 3rd case, which is actually the root of this discovery, is speculative
type confusion where the processor executes code expecting to use
{d,v}->arch.{pv,hvm}.$FOO, but is interpreting the data with the types
of the other union. For people familiar with Speculative Store Bypass
gadgets, this is the same kind of thing as the integer/pointer confusion
in that case.
The only viable fix for these is to avoid entering the basic block with
the vulnerable code pattern in the first place. I.e. "fixing" Spectre v1.
> Does it mean the CPU speculates over a direct call (assuming no #PF etc) and
> assumes some default return value to be used?
That wasn't the point I was trying to make, although Intel CPUs will
speculatively execute the instructions following an indirect call/jmp
while the frontend works out where to fetch from next.
The point was that, to avoid entering the wrong basic block, the lfence
must be later in the instruction stream than the conditional jump. The
frontend has to follow the attacker controlled jump, then serialise
itself to figure out if it went wrong.
Serialising first, then following the attacker controlled jump still
leaves a speculation window, which can execute the gadget.
> If not, maybe we should be more concerned about the value the cache-loading
> gadget speculates with, rather than the sheer speculation over the branch.
In a mythical world where a complete solution existed, channels other
than the data cache want considering. There is some interesting work
with instruction cache and TLB timing analysis, and these are far harder
to close.
Given the SpectreGS case of a bad base pointer, rather than a bad
offset, a non-spectre-v1 fix would have to clamp every register making
up part of a memory operand.
This is the approach which the Clang/LLVM Speculative Load Hardening
feature goes for, and comes with a 30% perf hit or so. Furthermore, the
current design of SLH is specific to userspace, and there is development
work needed to make it safe for kernel mode code.
Specifically, the muxing of the speculation token against registers
needs to turn from &= token to |= token, and the sense of the token
being 0 or -1 needs to reverse, because of kernel code operating in the
top of the address space, rather than the bottom. There is a further
complication given signed displacement. For kernel code, that can still
speculatively wrap around into userspace, and SMAP (on
meltdown-vulnerable parts) won't even halt speculation in this case.
Further further more, for Xen, that would still move incorrect
speculative memory accesses into PV guest kernel controlled space,
rather than Xen controlled space.
>
>> To protect against speculatively executing the wrong basic block, the
>> pipeline must execute the conditional jump first, *then* hit an lfence
>> to serialise the instruction stream and revector in the case of
>> incorrect speculation.
> That’s true, but there are also 2 aspects worth mentioning:
> 1) Example:
>
> jne 1
> PRIVILEGED
> 1:
> ALWAYS_SAFE
>
> We do not necessarily have to cover the 1: path with an lfence?
> We could allow speculation there, as it is harmless.
I agree, but how do you express that using evaluate_nospec()?
There is no information tying what is privileged and what is safe, to
the condition for entering the basic block.
> 2) cache-load protection
>
> It might be ok to speculate over a conditional branch, when we can
> guarantee that the value to be used in a dereference is not out-of-bound.
I agree (in principle, because the guarantee is implausible to make in
general case) but...
> In that case an lfence is used to latch the value in the register. We can
> still speculate over the branch and reach the dereference, but with a sane value.
... this is reasoning about the wrong basic block. This analogy is:
cmp ... #1
jcc 1f
ALWAYS_SAFE
lfence ("optimised" from the cmp #2 if statement)
cmp ... #2
jcc 1f
PRIVILEGED
1:
ALWAYS_SAFE
This is saying that the spilled lfence from cmp2 protects PRIVLEGED
because it might reduce the speculative variability in registers. There
are probably code sequences where that would be true, but there are
plenty of others where it would not be true.
This is because it is protecting cmp1's basic block (or at least, part
of it), not cmp2's. It will protect against an attack which requires
poisoning of both cmp1 and cmp2 to be function, but this is orders of
magnitude harder for the attacker to detect and arrange for, than an
attack which simply involves poisoning cmp2 to enter PRIVILEGED with an
architecturally-correct register state intended for ALWAYS_SAFE.
> I agree that lfence might not give us 100% security in every potential case,
> but that is what "hardening" gives you...
This being a best-effort approach is fine. There is no way it could
ever be called complete with a straight face.
If everything had been done using block_speculation(), that would also
be ok. There is no way the optimiser can break the safety there,
because it cannot reposition the lfence instruction relative to any
memory references (although the subtleties of it being able to be
repositioned relative to non-memory accesses still make it hard to
reason about in general).
The problem is that, at the moment, the optimiser is undermining the
safety which is attempting to be inserted by the use of evaluate_nospec().
We have code which appears to be safe at the C level, and isn't. A
false sense of security is arguably worse than no security.
The random sprinkling of lfences will reduce the amount of speculation,
but this is of very little use when it is only actually protecting
against certain more-complicated, and therefore rarer, gadgets but
leaves the common gadgets still exploitable.
~Andrew
[-- Attachment #1.2: Type: text/html, Size: 9818 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-02 19:31 ` Andrew Cooper
@ 2019-10-04 8:55 ` Jan Beulich
0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2019-10-04 8:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Xen-devel, Norbert Manthey, Wei Liu, Roger Pau Monné
On 02.10.2019 21:31, Andrew Cooper wrote:
> On 02/10/2019 09:24, Jan Beulich wrote:
>> On 01.10.2019 17:37, Andrew Cooper wrote:
>>> On 01/10/2019 15:32, Jan Beulich wrote:
>>>> On 01.10.2019 14:51, Andrew Cooper wrote:
>>>>> On 01/10/2019 13:21, Jan Beulich wrote:
>>>>>> On 30.09.2019 20:24, Andrew Cooper wrote:
>>>>>>> The code generation for barrier_nospec_true() is not correct. We are taking a
>>>>>>> perf it from the added fences, but not gaining any speculative safety.
>>>>>> You want to be more specific here, I think: ISTR you saying that the LFENCEs
>>>>>> get inserted at the wrong place.
>>>>> Correct.
>>>>>
>>>>>> IIRC we want them on either side of a
>>>>>> conditional branch, i.e. immediately following a branch insn as well as right
>>>>>> at the branch target.
>>>>> Specifically, they must be at the head of both basic blocks following
>>>>> the conditional jump.
>>>>>
>>>>>> I've taken, as a simple example,
>>>>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>>>>>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>>>>>> things to look like, or there's more to it than code generation simply being
>>>>>> "not correct".
>>>>> This example demonstrates the problem, and actually throws a further
>>>>> spanner in the works of how make this safe, which hadn't occurred to me
>>>>> before.
>>>>>
>>>>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>>>>> will look something like this:
>>>>>
>>>>> call p2m_mem_access_sanity_check
>>>>> ...
>>>>> lfence
>>>>> ...
>>>>> ret
>>>>> cmp $0, %eax
>>>>> jne ...
>>>>>
>>>>> Which is unsafe, because the only safe way to arrange this code would be:
>>>>>
>>>>> call p2m_mem_access_sanity_check
>>>>> ...
>>>>> ret
>>>>> cmp $0, %eax
>>>>> jne 1f
>>>>> lfence
>>>>> ...
>>>>> 1: lfence
>>>>> ...
>>>>>
>>>>> There is absolutely no possible way for inline assembly to be used to
>>>>> propagate this safety property across translation units. This is going
>>>>> to have to be an attribute of some form or another handled by the compiler.
>>>> But you realize that this particular example is basically a more
>>>> complex is_XYZ() check, which could be dealt with by inlining the
>>>> function. Of course there are going to be larger functions where
>>>> the result wants to be guarded like you say. But just like the
>>>> addition of the nospec macros to various is_XYZ() functions is a
>>>> manual operation (as long the compiler doesn't help), it would in
>>>> that case be a matter of latching the return value into a local
>>>> variable and using an appropriate guarding construct when
>>>> evaluating it.
>>> And this reasoning demonstrates yet another problem (this one was raised
>>> at the meeting in Chicago).
>>>
>>> evaluate_nospec() is not a useful construct if it needs inserting at
>>> every higher level predicate to result in safe code. This is
>>> boarderline-impossible to review for, and extremely easy to break
>>> accidentally.
>> I agree; since evaluate_nospec() insertion need is generally a hard
>> to investigate / review action, I don#t consider this unexpected.
>>
>>>> So I'm afraid for now I still can't agree with your "not correct"
>>>> assessment - the generated code in the example looks correct to
>>>> me, and if further guarding was needed in users of this particular
>>>> function, it would be those users which would need further
>>>> massaging.
>>> Safety against spectre v1 is not a matter of opinion.
>>>
>>> To protect against speculatively executing the wrong basic block, the
>>> pipeline must execute the conditional jump first, *then* hit an lfence
>>> to serialise the instruction stream and revector in the case of
>>> incorrect speculation.
>>>
>>> The other way around is not safe. Serialising the instruction stream
>>> doesn't do anything to protect against the attacker taking control of a
>>> later branch.
>>>
>>> The bigger problem is to do with classifying what we are protecting
>>> against. In the case of is_control_domain(), it is any action based on
>>> the result of the decision. For is_{pv,hvm}_domain(), is only (to a
>>> first approximation) speculative type confusion into the pv/hvm unions
>>> (which in practice extends to calling pv_/hvm_ functions as well).
>>>
>>> As for the real concrete breakages. In a staging build with GCC 6
>>>
>>> $ objdump -d xen-syms | grep '<is_hvm_domain>:' | wc -l
>>> 18
>>> $ objdump -d xen-syms | grep '<is_pv_domain>:' | wc -l
>>> 9
>>>
>>> All of which have the lfence too early to protect against speculative
>>> type confusion.
>> And all of which are because, other than I think it was originally
>> intended, the functions still aren't always_inline.
>
> Right, but if we force is_hvm_domain() to be always_inline, then
> is_hvm_vcpu() gets out-of-lined.
>
> This turns into a game of whack-a-mole, where any predicate wrapping
> something with an embedded evaluate_nospec() breaks the safety.
That's understood, but what do you do? The consequence is that we'd
have to go through _all_ inline (predicate) functions, converting
them to always_inline as needed. Auditing non-inline ones (like
p2m_mem_access_sanity_check()) would need doing independently anyway,
and I'd consider this an independent aspect/issue.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it
2019-10-02 21:02 ` Andrew Cooper
@ 2019-10-04 11:46 ` Wieczorkiewicz, Pawel
0 siblings, 0 replies; 23+ messages in thread
From: Wieczorkiewicz, Pawel @ 2019-10-04 11:46 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Wei Liu, Wieczorkiewicz, Pawel, Manthey, Norbert,
Jan Beulich, Xen-devel, Roger Pau Monné
> On 2. Oct 2019, at 23:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 02/10/2019 10:50, Wieczorkiewicz, Pawel wrote:
>>>>>> I've taken, as a simple example,
>>>>>> p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
>>>>>> code (in a non-debug build). Hence either I'm mis-remembering what we want
>>>>>> things to look like, or there's more to it than code generation simply being
>>>>>> "not correct".
>>>>>>
>>>>> This example demonstrates the problem, and actually throws a further
>>>>> spanner in the works of how make this safe, which hadn't occurred to me
>>>>> before.
>>>>>
>>>>> The instruction stream from a caller of p2m_mem_access_sanity_check()
>>>>> will look something like this:
>>>>>
>>>>> call p2m_mem_access_sanity_check
>>>>> ...
>>>>> lfence
>>>>> ...
>>>>> ret
>>>>> cmp $0, %eax
>>>>> jne ...
>>>>>
>>>>> Which is unsafe, because the only safe way to arrange this code would be:
>>>>>
>>>>> call p2m_mem_access_sanity_check
>>>>> ...
>>>>> ret
>>>>> cmp $0, %eax
>>>>> jne 1f
>>>>> lfence
>>>>> ...
>>>>> 1: lfence
>>>>> …
>>>>>
>>>>>
>
> Answering out of order, because I think this will make things clearer.
>
>> But the hardening wasn’t about spectre v1, but about cache-load gadgets?
>
> Ultimately, yes - the goal is cache load gadgets.
>
> Cache load gadgets are any memory read, where the attacker can influence the position of the load. The easy case to think about is the first half of a Spectre v1 gadget (i.e. the first memory load), but a second common case is a simple memory read with the wrong base pointer (as demonstrated clearly by SpectreGS and CVE-2019-1125).
>
Yes, that’s right.
> A 3rd case, which is actually the root of this discovery, is speculative type confusion where the processor executes code expecting to use {d,v}->arch.{pv,hvm}.$FOO, but is interpreting the data with the types of the other union. For people familiar with Speculative Store Bypass gadgets, this is the same kind of thing as the integer/pointer confusion in that case.
>
Yes, that’s right again. There is also a few other cases like abusing switch jump tables etc.
But, I would like to focus on the first half of a Spectre v1 gadgets.
> The only viable fix for these is to avoid entering the basic block with the vulnerable code pattern in the first place. I.e. "fixing" Spectre v1.
>
I think that’s not the only viable fix option. But, that depends on the basic block construction.
There certainly are basic block where this is the only viable fix. But there are also others.
Example 1:
mov (REG1), REG2
cmp $0x0,(REG3)
jne 1
mov (REG2), REG4 # Gadget load
1:
...
In the above case a lfence does not need to protect the branch speculation to kill off the cache-load gadget.
The lfence before the cmp instruction would make sure the REG2 holds an actual value.
Example 2: (probably a better one)
mov (REG1), REG2
cmp $0x0,REG2
jne 1
mov (REG3), REG4 # Gadget load
1:
...
When putting lfence before the cmp instruction, we limit the chances of speculation over the branch.
Same applies to this:
call is_allowed
mov (REG1), REG2
….
mov REG2, RAX
ret
cmp $0x0,RAX
jne 1
mov (REG2), REG4 # Gadget load
1:
...
Putting lfence before return from the function, so as to have the value for the cmp instruction fixed would be also good enough.
(I agree that might not be perfect, but just good enough…)
And finally:
Example 3:
cmp $0x0,(REG1)
jne 1
mov (REG2), REG3 # Gadget load
1:
…
Here indeed not much can be done and we have to put lfences the spectre v1 way.
>> Does it mean the CPU speculates over a direct call (assuming no #PF etc) and
>> assumes some default return value to be used?
>>
>
> That wasn't the point I was trying to make, although Intel CPUs will speculatively execute the instructions following an indirect call/jmp while the frontend works out where to fetch from next.
>
Yes, indirect call/jmp instruction could speculate, but that’s out of scope for this discussion.
> The point was that, to avoid entering the wrong basic block, the lfence must be later in the instruction stream than the conditional jump. The frontend has to follow the attacker controlled jump, then serialise itself to figure out if it went wrong.
>
Yes, that’s true. But, this is to avoid entering the wrong basic block. As I mentioned earlier that might not be the only option.
> Serialising first, then following the attacker controlled jump still leaves a speculation window, which can execute the gadget.
>
Yes, that’s also true. Such protection against entering a basic block does not guarantee anything.
One could argue that it limits the speculation window though. And, it makes it harder to orchestrate the attack.
But, I agree that this is a false sense of security.
>> If not, maybe we should be more concerned about the value the cache-loading
>> gadget speculates with, rather than the sheer speculation over the branch.
>>
>
> In a mythical world where a complete solution existed, channels other than the data cache want considering. There is some interesting work with instruction cache and TLB timing analysis, and these are far harder to close.
>
> Given the SpectreGS case of a bad base pointer, rather than a bad offset, a non-spectre-v1 fix would have to clamp every register making up part of a memory operand.
>
Yes, but only every register that could be influenced by an untrusted source.
That limits the overall number, but has its own challenges (taint analysis is hard as it is anyway…).
> This is the approach which the Clang/LLVM Speculative Load Hardening feature goes for, and comes with a 30% perf hit or so. Furthermore, the current design of SLH is specific to userspace, and there is development work needed to make it safe for kernel mode code.
>
Yes, I have spent some time with it. A very nice stuff but definitely still in the making.
> Specifically, the muxing of the speculation token against registers needs to turn from &= token to |= token, and the sense of the token being 0 or -1 needs to reverse, because of kernel code operating in the top of the address space, rather than the bottom. There is a further complication given signed displacement. For kernel code, that can still speculatively wrap around into userspace, and SMAP (on meltdown-vulnerable parts) won't even halt speculation in this case.
>
Yeah, I agree. That brings memories of XSA212. Good ol’ times ;-).
> Further further more, for Xen, that would still move incorrect speculative memory accesses into PV guest kernel controlled space, rather than Xen controlled space.
>
>>
>>> To protect against speculatively executing the wrong basic block, the
>>> pipeline must execute the conditional jump first, *then* hit an lfence
>>> to serialise the instruction stream and revector in the case of
>>> incorrect speculation.
>>>
>> That’s true, but there are also 2 aspects worth mentioning:
>> 1) Example:
>>
>> jne 1
>> PRIVILEGED
>> 1:
>> ALWAYS_SAFE
>>
>> We do not necessarily have to cover the 1: path with an lfence?
>> We could allow speculation there, as it is harmless.
>>
>
> I agree, but how do you express that using evaluate_nospec()?
>
> There is no information tying what is privileged and what is safe, to the condition for entering the basic block.
>
Yes, I think we all agree that trying to solve the general case with such mitigations might be infeasible and fragile.
But, despite that manual application of the mitigations demands a lot of effort and caution, I would argue that there
are places in code where it makes sense as a best-effort solution "for now".
>> 2) cache-load protection
>>
>> It might be ok to speculate over a conditional branch, when we can
>> guarantee that the value to be used in a dereference is not out-of-bound.
>>
>
> I agree (in principle, because the guarantee is implausible to make in general case) but…
>
Yes, I of course agree that there is absolutely no guarantee for general case.
And if there was a general case solution I would strongly recommend to use that instead.
>> In that case an lfence is used to latch the value in the register. We can
>> still speculate over the branch and reach the dereference, but with a sane value.
>>
>
> ... this is reasoning about the wrong basic block. This analogy is:
>
> cmp ... #1
> jcc 1f
> ALWAYS_SAFE
> lfence ("optimised" from the cmp #2 if statement)
> cmp ... #2
> jcc 1f
> PRIVILEGED
> 1:
> ALWAYS_SAFE
>
Well, the above code may be one or all of my examples (1,2, 3) from above.
> This is saying that the spilled lfence from cmp2 protects PRIVLEGED because it might reduce the speculative variability in registers. There are probably code sequences where that would be true, but there are plenty of others where it would not be true.
>
Totally agree with you here. I provided more thoughts for the examples above.
> This is because it is protecting cmp1's basic block (or at least, part of it), not cmp2's. It will protect against an attack which requires poisoning of both cmp1 and cmp2 to be function, but this is orders of magnitude harder for the attacker to detect and arrange for, than an attack which simply involves poisoning cmp2 to enter PRIVILEGED with an architecturally-correct register state intended for ALWAYS_SAFE.
>
Yes, that’s all true.
But, the actual incarnation of the above code, could be actually good enough iff the PRIVILEGED part does not do any memory dereferences with uncertain, attacker-influenced values (I have example 2 from above in mind here).
>> I agree that lfence might not give us 100% security in every potential case,
>> but that is what "hardening" gives you...
>>
>
> This being a best-effort approach is fine. There is no way it could ever be called complete with a straight face.
>
Totally agree again. It is definitely not even close to being complete. Better than nothing though.
> If everything had been done using block_speculation(), that would also be ok. There is no way the optimiser can break the safety there, because it cannot reposition the lfence instruction relative to any memory references (although the subtleties of it being able to be repositioned relative to non-memory accesses still make it hard to reason about in general).
>
Yes, that is still a band-aid mitigation, killing off most of the (more or less) obvious cache-load gadgets that manual effort can find.
General case is still open and I personally believe, that only compiler or silicon fixes could try to provide a complete, general enough
solution.
> The problem is that, at the moment, the optimiser is undermining the safety which is attempting to be inserted by the use of evaluate_nospec().
>
Yes. I have not see that myself (with GCC7), but I can imagine that could be easily the case.
> We have code which appears to be safe at the C level, and isn't. A false sense of security is arguably worse than no security.
>
Yes, I agree with such reasoning (if we are speaking of general case with evaluate_nospec()).
> The random sprinkling of lfences will reduce the amount of speculation, but this is of very little use when it is only actually protecting against certain more-complicated, and therefore rarer, gadgets but leaves the common gadgets still exploitable.
>
I agree in principle, but I do not think that this is the case with the patches in question (at least not with majority of the changes there).
> ~Andrew
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-10-04 11:46 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30 18:24 [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening Andrew Cooper
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_ARRAY_HARDEN Andrew Cooper
2019-10-01 10:45 ` Jan Beulich
2019-10-01 12:30 ` Andrew Cooper
2019-09-30 18:24 ` [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it Andrew Cooper
2019-09-30 20:17 ` Julien Grall
2019-09-30 23:21 ` Andrew Cooper
2019-10-01 9:17 ` Julien Grall
2019-10-01 9:22 ` Jan Beulich
2019-10-01 9:26 ` Julien Grall
2019-10-01 9:41 ` Andrew Cooper
2019-10-01 9:45 ` Julien Grall
2019-10-01 12:21 ` Jan Beulich
2019-10-01 12:51 ` Andrew Cooper
2019-10-01 14:32 ` Jan Beulich
2019-10-01 15:37 ` Andrew Cooper
2019-10-02 8:24 ` Jan Beulich
2019-10-02 19:31 ` Andrew Cooper
2019-10-04 8:55 ` Jan Beulich
2019-10-02 9:50 ` Wieczorkiewicz, Pawel
2019-10-02 21:02 ` Andrew Cooper
2019-10-04 11:46 ` Wieczorkiewicz, Pawel
2019-10-01 6:35 ` [Xen-devel] [PATCH for-4.13 0/2] xen/nospec: Add Kconfig options for speculative hardening Jürgen Groß
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.