Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>
> > > ---
> > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > CC: Wei Liu <wl@xxxxxxx>
> > > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > CC: Juergen Gross <jgross@xxxxxxxx>
> > > CC: Norbert Manthey <nmanthey@xxxxxxxxx>
> > > ---
> > >    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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel