| Message ID | 20191001143207.15844-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/nospec: Add Kconfig options for speculative hardening | expand |
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
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
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
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
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
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
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(+)