On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@xxxxxxx> 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@xxxxxxxxxxxxxxx >>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >>>>> --- >>>>> 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?