[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier at arm.com> wrote:
> On 02/05/17 15:50, Marc Zyngier wrote:
>> On 02/05/17 15:40, Catalin Marinas wrote:
>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>> to be used in the EL2 code, while distributions routinely compile their
>>>> kernel with it. We're just lucky that no code actually triggers the
>>>> instrumentation.
>>>>
>>>> Let's not try our luck for much longer, and disable stack-protector
>>>> for code living at EL2.
>>>>
>>>> Cc: stable at vger.kernel.org
>>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>>>> ---
>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>> @@ -2,6 +2,8 @@
>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>  #
>>>>
>>>> +ccflags-y += -fno-stack-protector
>>>> +
>>>
>>> While you are at it, should we have a -fpic here as well? The hyp code
>>> runs at a different location than the rest of the kernel.
>>
>> We definitely should. I've just tried this, and this doesn't seem to
>> work very well. At least this seems to break our jump label
>> implementation. I need to page in that part of the code base and see
>> what happens.
>
> So here's the issue:
>
>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
> In file included from ./include/linux/jump_label.h:120:0,
>                  from ./include/linux/dynamic_debug.h:5,
>                  from ./include/linux/printk.h:329,
>                  from ./include/linux/kernel.h:13,
>                  from ./include/asm-generic/bug.h:15,
>                  from ./arch/arm64/include/asm/bug.h:66,
>                  from ./include/linux/bug.h:4,
>                  from ./include/linux/mmdebug.h:4,
>                  from ./include/linux/mm.h:8,
>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>   asm goto("1: nop\n\t"
>   ^~~
> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>   asm goto("1: nop\n\t"
>   ^~~
> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>
> The corresponding code does this:
>
> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
> {
>         asm goto("1: nop\n\t"
>                  ".pushsection __jump_table,  \"aw\"\n\t"
>                  ".align 3\n\t"
>                  ".quad 1b, %l[l_yes], %c0\n\t"
>                  ".popsection\n\t"
>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>
>         return false;
> l_yes:
>         return true;
> }
>
> and the problem lies in the evaluation of "key", which probably
> cannot be guaranteed a constant at that point. There is also the
> issue that even if it was known, the branch cannot be easily
> patched in from the rest of the kernel (how is the l_yes address
> represented?).
>
> It looks to me like we either need to rewrite the whole of our
> static key infrastructure to cope with PIC, or switch over to
> the hyp_alternate_select() hack, which I'd rather avoid spreading
> further.
>
> In the end, I wonder if that's even worth it...
>

Could you check if it builds with

>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"

instead? We'd still need to update the code that interprets the
__jump_table fields, but it changes the references into relative ones,
which also reduces the size as a bonus.