aarch64: fix speculative execution past SVC vulnerability

  Even though SVC always causes a jump to another address, aarch64 CPUs
speculatively execute following instructions as if the SVC
instruction was not a jump instruction.
The speculative execution does not cross privilege-levels (to the jump
target as one would expect), but it continues on the kernel privilege
level as if the SVC instruction did not change the control flow -
thus execution anything that is accidentally linked after the SVC
instruction. Later, the results of this speculative execution are
always architecturally discarded, however they can leak data using
microarchitectural side channels. This speculative execution is very
reliable (seems to be unconditional) and it manages to complete even
relatively performance-heavy operations (e.g. multiple dependent
fetches from uncached memory).

Something like:
read(2)
...
[parse read result]
Gets potentially dangerous, since this would allow us potentially
reasonably reliably hit uninitialized memory (with potentially attacker
controlled offsets from prior reads) if we speculate over the SVC in the
syscall. Particularly in a netspectre-like scope.

Bugzilla record:
https://sourceware.org/bugzilla/show_bug.cgi?id=25436

Signed-off-by: Anthony Steinhauser <asteinhauser@google.com>
---
 sysdeps/unix/sysv/linux/aarch64/sysdep.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
  

* Anthony Steinhauser:

> Even though SVC always causes a jump to another address, aarch64 CPUs
> speculatively execute following instructions as if the SVC
> instruction was not a jump instruction.
> The speculative execution does not cross privilege-levels (to the jump
> target as one would expect), but it continues on the kernel privilege
> level as if the SVC instruction did not change the control flow -
> thus execution anything that is accidentally linked after the SVC
> instruction.

The PDF attached to the bug report says this:

| The speculative execution does not cross privilege-levels (to the jump
| target as one would expect), but it continues on the current privilege
| level as if those instructions did not change the control flow.

I assume that “current privilege level”  means “user mode”.

Which is it?  If it is possible to speculatively execute userspace code
in kernel mode, then a simple userspace code change will not fix this
because an attacker can simply use their own system call sequence.

Is there a performance benefit from inhibiting speculative execution?
It seems wasteful in many cases because it does take the side effects of
the system call into account.

Do we have to make this conditi onal on the CPU?  It wouldn't be fair to
penalize vendors who implement this correctly.  (As a general rule, I
really dislike software mitigation attempts because they are usually
incomplete and slow down execution for everyone, without actually fixing
any security issue.)

Thanks,
Florian
  

On 22/01/2020 01:29, Anthony Steinhauser wrote:
> Even though SVC always causes a jump to another address, aarch64 CPUs

> speculatively execute following instructions as if the SVC

> instruction was not a jump instruction.


thanks for the patch, it looks good, but i add
somebody on cc who spent more time on spectre
mitigation in case there is further insight.

> The speculative execution does not cross privilege-levels (to the jump

> target as one would expect), but it continues on the kernel privilege

> level as if the SVC instruction did not change the control flow -


i don't understand this bit: why is anything
executing on the kernel privilege level here?

> thus execution anything that is accidentally linked after the SVC

> instruction. Later, the results of this speculative execution are

> always architecturally discarded, however they can leak data using

> microarchitectural side channels. This speculative execution is very

> reliable (seems to be unconditional) and it manages to complete even

> relatively performance-heavy operations (e.g. multiple dependent

> fetches from uncached memory).

> 

> Something like:

> read(2)

> ...

> [parse read result]

> Gets potentially dangerous, since this would allow us potentially

> reasonably reliably hit uninitialized memory (with potentially attacker

> controlled offsets from prior reads) if we speculate over the SVC in the

> syscall. Particularly in a netspectre-like scope.

> 

> Bugzilla record:

> https://sourceware.org/bugzilla/show_bug.cgi?id=25436

> 

> Signed-off-by: Anthony Steinhauser <asteinhauser@google.com>

> ---

>  sysdeps/unix/sysv/linux/aarch64/sysdep.h | 8 ++++++--

>  1 file changed, 6 insertions(+), 2 deletions(-)

> 

> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h

> index 00b8e241c8..cf1e6ed45f 100644

> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h

> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h

> @@ -147,7 +147,9 @@

>  # undef	DO_CALL

>  # define DO_CALL(syscall_name, args)		\

>      mov x8, SYS_ify (syscall_name);		\

> -    svc 0

> +    svc 0;		\

> +    dsb nsh;		\

> +    isb

>  

>  #else /* not __ASSEMBLER__ */

>  

> @@ -191,7 +193,9 @@

>       {								\

>         LOAD_ARGS_##nr (args)					\

>         register long _x8 asm ("x8") = (name);			\

> -       asm volatile ("svc	0	// syscall " # name     \

> +       asm volatile ("svc	0	// syscall\n\t" # name  \

> +		     "dsb	nsh\n\t"				\

> +		     "isb"					\

>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\

>         _sys_result = _x0;					\

>       }								\

>
  

On 22/01/2020 10:08, Szabolcs Nagy wrote:
> On 22/01/2020 01:29, Anthony Steinhauser wrote:

>>       {								\

>>         LOAD_ARGS_##nr (args)					\

>>         register long _x8 asm ("x8") = (name);			\

>> -       asm volatile ("svc	0	// syscall " # name     \

>> +       asm volatile ("svc	0	// syscall\n\t" # name  \


\n before the name looks wrong

>> +		     "dsb	nsh\n\t"				\

>> +		     "isb"					\

>>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\

>>         _sys_result = _x0;					\

>>       }								\
  

*Florian Weimer
It's the user mode in the case of SVC instruction. I incorrectly
copied that passage from the ERET kernel fixes. Sorry for that.
If there are vendors or CPUs who implement this correctly, it would be
good to make it CPU-specific.
On the ARM and Cavium CPUs that I tested the control flow past SVC is
always mispredicted, so it seems like the barrier there would have
little if any overhead.
However, it does not enhance the performance either. It just prevents
very similar Spectre-like effects as in the ERET case.

*Szabolcs Nagy
I'm not sure whether the endline should be wrong. Without it, the
resulting assembly would look like:
svc 0 // syscall dsb nsh
isb
So the dsb nsh instruction will be commented-out. Or am I missing something?


On Wed, Jan 22, 2020 at 2:12 AM Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
>
> On 22/01/2020 10:08, Szabolcs Nagy wrote:
> > On 22/01/2020 01:29, Anthony Steinhauser wrote:
> >>       {                                                              \
> >>         LOAD_ARGS_##nr (args)                                        \
> >>         register long _x8 asm ("x8") = (name);                       \
> >> -       asm volatile ("svc   0       // syscall " # name     \
> >> +       asm volatile ("svc   0       // syscall\n\t" # name  \
>
> \n before the name looks wrong
>
> >> +                 "dsb       nsh\n\t"                                \
> >> +                 "isb"                                      \
> >>                   : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \
> >>         _sys_result = _x0;                                   \
> >>       }                                                              \
  

On Wed, Jan 22, 2020 at 12:39 PM Anthony Steinhauser
<asteinhauser@google.com> wrote:
>
> *Florian Weimer
> It's the user mode in the case of SVC instruction. I incorrectly
> copied that passage from the ERET kernel fixes. Sorry for that.
> If there are vendors or CPUs who implement this correctly, it would be
> good to make it CPU-specific.
> On the ARM and Cavium CPUs that I tested the control flow past SVC is
> always mispredicted, so it seems like the barrier there would have
> little if any overhead.
> However, it does not enhance the performance either. It just prevents
> very similar Spectre-like effects as in the ERET case.
>
> *Szabolcs Nagy
> I'm not sure whether the endline should be wrong. Without it, the
> resulting assembly would look like:
> svc 0 // syscall dsb nsh
> isb
> So the dsb nsh instruction will be commented-out. Or am I missing something?

He means the return (\n) is misplaced.  It should be after the #name.
e.g.
asm volatile ("svc   0       // syscall" # name "\n\t"

Thanks,
Andrew Pinski

>
>
> On Wed, Jan 22, 2020 at 2:12 AM Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
> >
> > On 22/01/2020 10:08, Szabolcs Nagy wrote:
> > > On 22/01/2020 01:29, Anthony Steinhauser wrote:
> > >>       {                                                              \
> > >>         LOAD_ARGS_##nr (args)                                        \
> > >>         register long _x8 asm ("x8") = (name);                       \
> > >> -       asm volatile ("svc   0       // syscall " # name     \
> > >> +       asm volatile ("svc   0       // syscall\n\t" # name  \
> >
> > \n before the name looks wrong
> >
> > >> +                 "dsb       nsh\n\t"                                \
> > >> +                 "isb"                                      \
> > >>                   : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \
> > >>         _sys_result = _x0;                                   \
> > >>       }                                                              \
  

I get it. Sorry for the confusion.

On Wed, Jan 22, 2020 at 12:43 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Wed, Jan 22, 2020 at 12:39 PM Anthony Steinhauser
> <asteinhauser@google.com> wrote:
> >
> > *Florian Weimer
> > It's the user mode in the case of SVC instruction. I incorrectly
> > copied that passage from the ERET kernel fixes. Sorry for that.
> > If there are vendors or CPUs who implement this correctly, it would be
> > good to make it CPU-specific.
> > On the ARM and Cavium CPUs that I tested the control flow past SVC is
> > always mispredicted, so it seems like the barrier there would have
> > little if any overhead.
> > However, it does not enhance the performance either. It just prevents
> > very similar Spectre-like effects as in the ERET case.
> >
> > *Szabolcs Nagy
> > I'm not sure whether the endline should be wrong. Without it, the
> > resulting assembly would look like:
> > svc 0 // syscall dsb nsh
> > isb
> > So the dsb nsh instruction will be commented-out. Or am I missing something?
>
> He means the return (\n) is misplaced.  It should be after the #name.
> e.g.
> asm volatile ("svc   0       // syscall" # name "\n\t"
>
> Thanks,
> Andrew Pinski
>
> >
> >
> > On Wed, Jan 22, 2020 at 2:12 AM Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
> > >
> > > On 22/01/2020 10:08, Szabolcs Nagy wrote:
> > > > On 22/01/2020 01:29, Anthony Steinhauser wrote:
> > > >>       {                                                              \
> > > >>         LOAD_ARGS_##nr (args)                                        \
> > > >>         register long _x8 asm ("x8") = (name);                       \
> > > >> -       asm volatile ("svc   0       // syscall " # name     \
> > > >> +       asm volatile ("svc   0       // syscall\n\t" # name  \
> > >
> > > \n before the name looks wrong
> > >
> > > >> +                 "dsb       nsh\n\t"                                \
> > > >> +                 "isb"                                      \
> > > >>                   : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \
> > > >>         _sys_result = _x0;                                   \
> > > >>       }                                                              \
  

* Anthony Steinhauser <asteinhauser@google.com> [2020-01-22 12:39:37 -0800]:
> *Florian Weimer
> It's the user mode in the case of SVC instruction. I incorrectly
> copied that passage from the ERET kernel fixes. Sorry for that.

please write a commit message that has enough
detail so we understand why you want to fix
this particular user mode speculation issue
if there is no privilege escalation.

> If there are vendors or CPUs who implement this correctly, it would be
> good to make it CPU-specific.
> On the ARM and Cavium CPUs that I tested the control flow past SVC is
> always mispredicted, so it seems like the barrier there would have
> little if any overhead.
> However, it does not enhance the performance either. It just prevents
> very similar Spectre-like effects as in the ERET case.
> 
> *Szabolcs Nagy
> I'm not sure whether the endline should be wrong. Without it, the
> resulting assembly would look like:
> svc 0 // syscall dsb nsh
> isb
> So the dsb nsh instruction will be commented-out. Or am I missing something?
> 
> 
> On Wed, Jan 22, 2020 at 2:12 AM Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
> >
> > On 22/01/2020 10:08, Szabolcs Nagy wrote:
> > > On 22/01/2020 01:29, Anthony Steinhauser wrote:
> > >>       {                                                              \
> > >>         LOAD_ARGS_##nr (args)                                        \
> > >>         register long _x8 asm ("x8") = (name);                       \
> > >> -       asm volatile ("svc   0       // syscall " # name     \
> > >> +       asm volatile ("svc   0       // syscall\n\t" # name  \
> >
> > \n before the name looks wrong
> >
> > >> +                 "dsb       nsh\n\t"                                \
> > >> +                 "isb"                                      \
> > >>                   : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \
> > >>         _sys_result = _x0;                                   \
> > >>       }                                                              \