Re: [Xen-devel] [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 25/01/18 18:45, Stefano Stabellini wrote:
> On Thu, 25 Jan 2018, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 24/01/18 23:54, Stefano Stabellini wrote:
> > > On Fri, 19 Jan 2018, Julien Grall wrote:
> > > > Aliasing attacked against CPU branch predictors can allow an attacker to
> > > > redirect speculative control flow on some CPUs and potentially divulge
> > > > information from one context to another.
> > > >
> > > > This patch adds initiatial skeleton code behind a new Kconfig option
> > > > to enable implementation-specific mitigations against these attacks
> > > > for CPUs that are affected.
> > > >
> > > > Most of mitigations will have to be applied when entering to the
> > > > hypervisor from the guest context.
> > > >
> > > > Because the attack is against branch predictor, it is not possible to
> > > > safely use branch instruction before the mitigation is applied.
> > > > Therefore this has to be done in the vector entry before jump to the
> > > > helper handling a given exception.
> > > >
> > > > However, on arm32, each vector contain a single instruction. This means
> > > > that the hardened vector tables may rely on the state of registers that
> > > > does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
> > > > Therefore hypervisor code running with guest vectors table should be
> > > > minimized and always have interrupts masked to reduce the risk to use
> > > > them.
> > > >
> > > > This patch provides an infrastructure to switch vector tables before
> > > > entering to the guest and when leaving it.
> > > >
> > > > Note that alternative could have been used, but older Xen (4.8 or
> > > > earlier) doesn't have support. So avoid using alternative to ease
> > > > backporting.
> > > >
> > > > This is part of XSA-254.
> > > >
> > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > > I only have a couple of questions. It looks good.
> > >
> > >
> > > > ---
> > > >    xen/arch/arm/Kconfig       |  3 +++
> > > >    xen/arch/arm/arm32/entry.S | 41
> > > > ++++++++++++++++++++++++++++++++++++++++-
> > > >    xen/arch/arm/cpuerrata.c   | 30 ++++++++++++++++++++++++++++++
> > > >    3 files changed, 73 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > index 06fd85cc77..2782ee6589 100644
> > > > --- a/xen/arch/arm/Kconfig
> > > > +++ b/xen/arch/arm/Kconfig
> > > > @@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
> > > >    config ARM64_HARDEN_BRANCH_PREDICTOR
> > > >        def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
> > > >    +config ARM32_HARDEN_BRANCH_PREDICTOR
> > > > +    def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
> > > > +
> > > >    source "common/Kconfig"
> > > >      source "drivers/Kconfig"
> > > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> > > > index c2fad5fe9b..54a1733f87 100644
> > > > --- a/xen/arch/arm/arm32/entry.S
> > > > +++ b/xen/arch/arm/arm32/entry.S
> > > > @@ -34,6 +34,20 @@
> > > >            blne save_guest_regs
> > > >      save_guest_regs:
> > > > +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> > > > +        /*
> > > > +         * Restore vectors table to the default as it may have been
> > > > +         * changed when returning to the guest (see
> > > > +         * return_to_hypervisor). We need to do that early (e.g before
> > > > +         * any interrupts are unmasked) because hardened vectors requires
> > > > +         * SP to be 8 bytes aligned. This does not hold when running in
> > > > +         * the hypervisor.
> > > > +         */
> > > > +        ldr r1, =hyp_traps_vector
> > > > +        mcr p15, 4, r1, c12, c0, 0
> > > > +        isb
> > > > +#endif
> > > > +
> > > >            ldr r11, =0xffffffff  /* Clobber SP which is only valid for
> > > > hypervisor frames. */
> > > >            str r11, [sp, #UREGS_sp]
> > > >            SAVE_ONE_BANKED(SP_usr)
> > > > @@ -179,12 +193,37 @@ return_to_guest:
> > > >            RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
> > > >            /* Fall thru */
> > > >    return_to_hypervisor:
> > > > -        cpsid i
> > > > +        cpsid ai
> > > Why?
> > Asynchronous abort is a kind of interrupt, as we are going to switch to the
> > hardened vector tables you don't want an interrupt to come up.
> >
> > This is because the hardened vector tables requires SP to be 8 bytes aligned.
> > When in the hypervisor, and particularly when restoring the register (as
> > below), this assumption does not hold.
> >
> > So masking all interrupts (as mentioned a few time within the patch and the
> > commit message) will reduce the risk to use the hardened vectors. I say reduce
> > because you may have receive data abort (imagine an unlikely error in the few
> > instructions to restore state).
> >
> > It is also why switching vector tables are done very early in entry path and
> > very late in the exit path.
> All right, thanks for the explanation. Please add "and mask asynchronous
> aborts" in the commit message.
I am not surely what you exactly suggest here. The commit message 
currently contains:
"Therefore hypervisor code running with guest vectors table should be
minimized and always have interrupts masked to reduce the risk to use
them."

What are you suggesting?

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel