Re: [PATCH 2/2] xen/arm: Mitigate straight-line speculation for SMC call

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Tue, 16 Jun 2020 at 22:34, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>
> On Tue, 16 Jun 2020, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > SMC call will update some of registers (typically only x0) depending on
>   ^a SMC call
>
> > the arguments provided.
> >
> > Some CPUs can speculate past a SMC instruction and potentially perform
> > speculative access to emrmoy using the pre-call values before executing
>                         ^ memory
>
> > the SMC.
> >
> > There is no known gadget available after the SMC call today. However
> > some of the registers may contain values from the guest and are expected
> > to be updated by the SMC call.
> >
> > In order to harden the code, it would be better to prevent straight-line
> > speculation from an SMC. Architecturally executing the speculation
>                    ^ a? any?

"any" might be better.

>
>
> > barrier after every SMC can be rather expensive (particularly on core
> > not SB). Therefore we want to mitigate it diferrently:
>        ^ not SB capable?                    ^ differently

It might be better to say "which doesn't support ARMv8.0-SB"

> >   */
> >  #define arm_smccc_1_1_smc(...)                                  \
> >      do {                                                        \
> >          __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> >          asm volatile("smc #0\n"                                 \
> > +                     "b 1f\n"                                   \
> > +                     ASM_SB                                     \
> > +                     "1:\n"                                     \
> >                       __constraints(__count_args(__VA_ARGS__))); \
> >          if ( ___res )                                           \
> >          *___res = (typeof(*___res)){r0, r1, r2, r3};            \
> > diff --git a/xen/include/asm-arm/system.h b/xen/include/asm-arm/system.h
> > index 65d5c8e423d7..e33ff4e0fc39 100644
> > --- a/xen/include/asm-arm/system.h
> > +++ b/xen/include/asm-arm/system.h
> > @@ -33,6 +33,14 @@
> >  #define smp_mb__before_atomic()    smp_mb()
> >  #define smp_mb__after_atomic()     smp_mb()
> >
> > +/*
> > + * Speculative barrier
> > + * XXX: Add support for the 'sb' instruction
> > + */
> > +#define ASM_SB "dsb nsh \n isb \n"
>
> Why not ';' ? Anyway it doesn't matter.

Per [1] and [2], a semicolon is not portable as some assemblers may
treat anything after it as a comment.

This reminds me that I have been using semicolons in entry.S. I
should probably have a look to avoid them.

Cheers,

[1] 
https://developer.arm.com/docs/dui0801/d/structure-of-assembly-language-modules/syntax-of-source-lines-in-assembly-language
[2] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate