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

[Julien Grall <julien.grall@linaro.org>]

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@linaro.org>
>>>
>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel