Date: Fri, 4 May 2018 11:30:31 +0300 From: Alexander Popov <alex.popov@...ux.com> To: Laura Abbott <labbott@...hat.com>, Mark Rutland <mark.rutland@....com> Cc: Kees Cook <keescook@...omium.org>, Ard Biesheuvel <ard.biesheuvel@...aro.org>, kernel-hardening@...ts.openwall.com, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 2/2] arm64: Clear the stack On 03.05.2018 22:09, Laura Abbott wrote: > On 05/03/2018 10:33 AM, Alexander Popov wrote: >> On 03.05.2018 10:19, Mark Rutland wrote: >>> On Wed, May 02, 2018 at 01:33:26PM -0700, Laura Abbott wrote: >>>> + /* Reset the lowest_stack value for the next syscall */ >>>> + current->thread.lowest_stack = current_stack_pointer; >> >> Laura, that might be wrong and introduce huge performance impact. >> >> I think, lowest_stack should be reset similarly to the original version. >> > > Sorry, I'm not understanding here. What's the performance impact and > what do you mean by original version? I meant the code for x86: /* Reset the lowest_stack value for the next syscall */ current->thread.lowest_stack = current_top_of_stack() - 256; ...Now when I'm writing about the performance impact, I see that I was wrong about "huge". Excuse me. Let me describe the implications of this code change. So we are at the end of a syscall. We've just erased the used part of the kernel stack. The current stack pointer is near to the top of stack. On x86_64 I see that the stack pointer is stack top minus 56 bytes (just before switching onto the trampoline stack). I took the idea of resetting lowest_stack to stack top minus 256 from the original PaX Team's code. It should give the speedup when lowest_stack is not updated during a syscall (a lot of functions are not instrumented) and we start to search for the poison value from that reasonable point. If we speak about the common erase_kstack() code, this code change can break x86, because this function can be called from the trampoline stack (separate from the thread stack). >>>> +} >>> >>> Once this function returns, its data is left on the stack. Is that not a problem? >>> >>> No strong feelings either way, but it might be worth mentioning in the commit >>> message. >> >> I managed to bypass that with "register" specifier. Although it doesn't give an >> absolute guarantee. >> > > I guess I was assuming gcc would be smart enough not to spill stuff > on the stack. I also intentionally removed the register keyword > since it wasn't clear gcc does much with it on a modern system? I > could be completely off base here though so please correct me if > I'm wrong. It probably is worth documenting what we are assuming about > the compiler here. I think having register storage class specifier here is a bit better than nothing. And yes, I'll add a comment. Right now don't see a better solution. >>>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile >>>> index a34e9290a699..25dd2a14560d 100644 >>>> --- a/drivers/firmware/efi/libstub/Makefile >>>> +++ b/drivers/firmware/efi/libstub/Makefile >>>> @@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt >>>> KBUILD_CFLAGS := $(cflags-y) -DDISABLE_BRANCH_PROFILING \ >>>> -D__NO_FORTIFY \ >>>> $(call cc-option,-ffreestanding) \ >>>> - $(call cc-option,-fno-stack-protector) >>>> + $(call cc-option,-fno-stack-protector) \ >>>> + $(DISABLE_STACKLEAK_PLUGIN) >>>> >>>> GCOV_PROFILE := n >>>> KASAN_SANITIZE := n >>> >>> I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/. >> >> Could you please give more details on that? Why STACKLEAK breaks it? >> > > For reference, I originally added this for the efistub because > it would not compile. I guess it was a linkage error, right? > I did compile this against my Fedora tree which has KVM enabled. Looked through this big article about ARM, KVM and HYP mode: https://lwn.net/Articles/557132/ So we have some limited amount of kernel code which runs in HYP mode. Is it only in arch/arm64/kvm/hyp/ directory? Mark, could you give a clue what trouble will we have if we call track_stack() or check_alloca() from that code? Thanks in advance! -- Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.