[v2,1/2] xen/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_ARRAY
diff mbox series

Message ID 20191001143207.15844-2-andrew.cooper3@citrix.com
State New
Headers show
Series
  • xen/nospec: Add Kconfig options for speculative hardening
Related show

Commit Message

Andrew Cooper Oct. 1, 2019, 2:32 p.m. UTC
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>
Release-acked-by: Juergen Gross <jgross@suse.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>

v2:
 * Rename to CONFIG_SPECULATIVE_HARDEN_ARRAY
 * Simplify the stub array_index_nospec()
---
 xen/common/Kconfig       | 24 ++++++++++++++++++++++++
 xen/include/xen/nospec.h |  5 +++++
 2 files changed, 29 insertions(+)

Comments

Jan Beulich Oct. 1, 2019, 2:48 p.m. UTC | #1
On 01.10.2019 16:32, Andrew Cooper wrote:
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one more question (I'm sorry, I meant to ask on v1 but then
forgot):

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -77,6 +77,30 @@ config HAS_CHECKPOLICY
>  	string
>  	option env="XEN_HAS_CHECKPOLICY"
>  
> +menu "Speculative hardening"
> +
> +config SPECULATIVE_HARDEN_ARRAY
> +	bool "Speculative Array Hardening"
> +	default y

Are you/we convinced it is a good idea to expose this without EXPERT
qualifier? I know you dislike that entire model, but our common
grounds still are - afaict - that we don't want a proliferation of
(security) supported configuration variations.

Jan
Andrew Cooper Oct. 1, 2019, 3:52 p.m. UTC | #2
On 01/10/2019 15:48, Jan Beulich wrote:
> On 01.10.2019 16:32, Andrew Cooper wrote:
>> 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one more question (I'm sorry, I meant to ask on v1 but then
> forgot):
>
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -77,6 +77,30 @@ config HAS_CHECKPOLICY
>>  	string
>>  	option env="XEN_HAS_CHECKPOLICY"
>>  
>> +menu "Speculative hardening"
>> +
>> +config SPECULATIVE_HARDEN_ARRAY
>> +	bool "Speculative Array Hardening"
>> +	default y
> Are you/we convinced it is a good idea to expose this without EXPERT
> qualifier? I know you dislike that entire model, but our common
> grounds still are - afaict - that we don't want a proliferation of
> (security) supported configuration variations.

Its not EXPERT I dislike.  Having a CONFIG_EXPERT just like Linux has
would be fine.  Its the fact that it will silently revert behind your
back if an environment variable is missing which is what makes the
behaviour toxic for people to use.

That aside, I don't think this warrants expert.  It is best-effort-only
mitigation, which on the balance of probability is not complete, which
can safely be turned off based on a risk assessment of the target CPU
and environment.

~Andrew
Jan Beulich Oct. 1, 2019, 3:58 p.m. UTC | #3
On 01.10.2019 17:52, Andrew Cooper wrote:
> On 01/10/2019 15:48, Jan Beulich wrote:
>> On 01.10.2019 16:32, Andrew Cooper wrote:
>>> 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>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with one more question (I'm sorry, I meant to ask on v1 but then
>> forgot):
>>
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -77,6 +77,30 @@ config HAS_CHECKPOLICY
>>>  	string
>>>  	option env="XEN_HAS_CHECKPOLICY"
>>>  
>>> +menu "Speculative hardening"
>>> +
>>> +config SPECULATIVE_HARDEN_ARRAY
>>> +	bool "Speculative Array Hardening"
>>> +	default y
>> Are you/we convinced it is a good idea to expose this without EXPERT
>> qualifier? I know you dislike that entire model, but our common
>> grounds still are - afaict - that we don't want a proliferation of
>> (security) supported configuration variations.
> 
> Its not EXPERT I dislike.  Having a CONFIG_EXPERT just like Linux has
> would be fine.  Its the fact that it will silently revert behind your
> back if an environment variable is missing which is what makes the
> behaviour toxic for people to use.
> 
> That aside, I don't think this warrants expert.  It is best-effort-only
> mitigation, which on the balance of probability is not complete, which
> can safely be turned off based on a risk assessment of the target CPU
> and environment.

I mostly agree with this; the question though was more towards whether
this is a good enough reason to set a(nother) precedent of an EXPERT-
less option, when we try to have as few of them as possible.

Jan
Andrew Cooper Oct. 1, 2019, 4:16 p.m. UTC | #4
On 01/10/2019 16:58, Jan Beulich wrote:
> On 01.10.2019 17:52, Andrew Cooper wrote:
>> On 01/10/2019 15:48, Jan Beulich wrote:
>>> On 01.10.2019 16:32, Andrew Cooper wrote:
>>>> 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>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> with one more question (I'm sorry, I meant to ask on v1 but then
>>> forgot):
>>>
>>>> --- a/xen/common/Kconfig
>>>> +++ b/xen/common/Kconfig
>>>> @@ -77,6 +77,30 @@ config HAS_CHECKPOLICY
>>>>  	string
>>>>  	option env="XEN_HAS_CHECKPOLICY"
>>>>  
>>>> +menu "Speculative hardening"
>>>> +
>>>> +config SPECULATIVE_HARDEN_ARRAY
>>>> +	bool "Speculative Array Hardening"
>>>> +	default y
>>> Are you/we convinced it is a good idea to expose this without EXPERT
>>> qualifier? I know you dislike that entire model, but our common
>>> grounds still are - afaict - that we don't want a proliferation of
>>> (security) supported configuration variations.
>> Its not EXPERT I dislike.  Having a CONFIG_EXPERT just like Linux has
>> would be fine.  Its the fact that it will silently revert behind your
>> back if an environment variable is missing which is what makes the
>> behaviour toxic for people to use.
>>
>> That aside, I don't think this warrants expert.  It is best-effort-only
>> mitigation, which on the balance of probability is not complete, which
>> can safely be turned off based on a risk assessment of the target CPU
>> and environment.
> I mostly agree with this; the question though was more towards whether
> this is a good enough reason to set a(nother) precedent of an EXPERT-
> less option, when we try to have as few of them as possible.

Remember that it is only you who is striving to have 0 EXPERT-less
options.  It is not a view shared by everyone, and is certainly not a
stated goal of our Kconfig setup.

This is an option which is safe to flip, and will want to be flipped by
users in some circumstances.  Hiding it doesn't seem like a reasonable
thing to do.

~Andrew
Jan Beulich Oct. 2, 2019, 8:15 a.m. UTC | #5
On 01.10.2019 18:16, Andrew Cooper wrote:
> On 01/10/2019 16:58, Jan Beulich wrote:
>> On 01.10.2019 17:52, Andrew Cooper wrote:
>>> On 01/10/2019 15:48, Jan Beulich wrote:
>>>> On 01.10.2019 16:32, Andrew Cooper wrote:
>>>>> 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>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> with one more question (I'm sorry, I meant to ask on v1 but then
>>>> forgot):
>>>>
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -77,6 +77,30 @@ config HAS_CHECKPOLICY
>>>>>  	string
>>>>>  	option env="XEN_HAS_CHECKPOLICY"
>>>>>  
>>>>> +menu "Speculative hardening"
>>>>> +
>>>>> +config SPECULATIVE_HARDEN_ARRAY
>>>>> +	bool "Speculative Array Hardening"
>>>>> +	default y
>>>> Are you/we convinced it is a good idea to expose this without EXPERT
>>>> qualifier? I know you dislike that entire model, but our common
>>>> grounds still are - afaict - that we don't want a proliferation of
>>>> (security) supported configuration variations.
>>> Its not EXPERT I dislike.  Having a CONFIG_EXPERT just like Linux has
>>> would be fine.  Its the fact that it will silently revert behind your
>>> back if an environment variable is missing which is what makes the
>>> behaviour toxic for people to use.
>>>
>>> That aside, I don't think this warrants expert.  It is best-effort-only
>>> mitigation, which on the balance of probability is not complete, which
>>> can safely be turned off based on a risk assessment of the target CPU
>>> and environment.
>> I mostly agree with this; the question though was more towards whether
>> this is a good enough reason to set a(nother) precedent of an EXPERT-
>> less option, when we try to have as few of them as possible.
> 
> Remember that it is only you who is striving to have 0 EXPERT-less
> options.  It is not a view shared by everyone, and is certainly not a
> stated goal of our Kconfig setup.

"Only you" is definitely too narrow. Back when this was discussed, I
definitely wasn't the only one concerned of people reporting issues
with arbitrary configurations they'd deem sensible. If it really was
only me, then I would shut up, but probably leave you and others pick
up the pieces.

Jan

Patch
diff mbox series

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 16829f6274..911333357a 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -77,6 +77,30 @@  config HAS_CHECKPOLICY
 	string
 	option env="XEN_HAS_CHECKPOLICY"
 
+menu "Speculative hardening"
+
+config SPECULATIVE_HARDEN_ARRAY
+	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.
+
+	  This is a best-effort mitigation.  There are no guarantees that all
+	  areas of code open to abuse have been hardened.
+
+	  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..76255bc46e 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_HARDEN_ARRAY
 /*
  * array_index_nospec - sanitize an array index after a bounds check
  *
@@ -58,6 +59,10 @@  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) ((void)(size), (index))
+#endif /* CONFIG_SPECULATIVE_HARDEN_ARRAY */
 
 /*
  * array_access_nospec - allow nospec access for static size arrays