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

[Ard Biesheuvel]

On 11 May 2017 at 17:42, Marc Zyngier <marc.zyngier at arm.com> wrote:
> On 11/05/17 17:36, Ard Biesheuvel wrote:
>> On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
>>> 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.
>>
>> OK, strike that, this is more tricky than I thought. I am failing to
>> reproduce this locally, though. Which gcc and tree are you using?
>
> That's current mainline + a number of patches which I don't think are
> relevant to this discussion, and -fPIC added to
> arch/arm64/kvm/hyp/Makefile. You should see it exploding in timer-sr.c
> because of the has_vhe() helper.
>
> GCC is "aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016".
>

Nope, builds fine, with Linaro GCC 5.4.0 and 'ccflags-y += -fPIC'
added to arch/arm64/kvm/hyp/Makefile.

In any case, it is worth trying whether -fpie behaves differently: as
per my other reply, aarch64 small model code is already mostly
position independent anyway, and so -fpic (which is intended for
dynamic linking under ELF preemption rules*) is more likely to emit
absolute symbol references than ordinary code. -fpie is supposed to be
the middle ground here, but I dismissed it for the EFI stub because I
could not get it to work at the time.

*) Preemption in ELF means any externally visible symbol can be
overridden by the main executable, in which case the shared library
must update all its internal references as well. In this particular
case, if the key argument to arch_static_branch() refers to a static
key that is part of an externally visible structure, its address is
preemptible at load time, which I suspect may be causing the error you
are seeing.