[lkml]   [2018]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [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
>>>> -D__NO_FORTIFY \
>>>> $(call cc-option,-ffreestanding) \
>>>> - $(call cc-option,-fno-stack-protector)
>>>> + $(call cc-option,-fno-stack-protector) \
>>>> GCOV_PROFILE := 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:

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!

 \ /
  Last update: 2018-05-04 10:31    [W:0.073 / U:0.552 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site