[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <71199506-b46b-5f91-e489-e6450b6d1067@linux.com>
Date: Fri, 11 May 2018 18:50:09 +0300
From: Alexander Popov <alex.popov@...ux.com>
To: Mark Rutland <mark.rutland@....com>, Andy Lutomirski <luto@...nel.org>,
Laura Abbott <labbott@...hat.com>, Kees Cook <keescook@...omium.org>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc: 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
Hello everyone,
On 06.05.2018 11:22, Alexander Popov wrote:
> On 04.05.2018 14:09, Mark Rutland wrote:
>>>>> + stack_left = sp & (THREAD_SIZE - 1);
>>>>> + BUG_ON(stack_left < 256 || size >= stack_left - 256);
>>>>
>>>> Is this arbitrary, or is there something special about 256?
>>>>
>>>> Even if this is arbitrary, can we give it some mnemonic?
>>>
>>> It's just a reasonable number. We can introduce a macro for it.
>>
>> I'm just not sure I see the point in the offset, given things like
>> VMAP_STACK exist. BUG_ON() handling will likely require *more* than 256
>> bytes of stack, so it seems superfluous, as we'd be relying on stack
>> overflow detection at that point.
>>
>> I can see that we should take the CONFIG_SCHED_STACK_END_CHECK offset
>> into account, though.
>
> Mark, thank you for such an important remark!
>
> In Kconfig STACKLEAK implies but doesn't depend on VMAP_STACK. In fact x86_32
> doesn't have VMAP_STACK at all but can have STACKLEAK.
>
> [Adding Andy Lutomirski]
>
> I've made some additional experiments: I exhaust the thread stack to have only
> (MIN_STACK_LEFT - 1) bytes left and then force alloca. If VMAP_STACK is
> disabled, BUG_ON() handling causes stack depth overflow, which is detected by
> SCHED_STACK_END_CHECK. If VMAP_STACK is enabled, the kernel hangs on BUG_ON()
> handling! Enabling CONFIG_PROVE_LOCKING gives the needed report from VMAP_STACK:
[...]
> I can't say why VMAP_STACK report hangs during BUG_ON() handling on defconfig.
> Andy, can you give a clue?
>
> I see that MIN_STACK_LEFT = 2048 is enough for BUG_ON() handling on both x86_64
> and x86_32. So I'm going to:
> - set MIN_STACK_LEFT to 2048;
> - improve the lkdtm test to cover this case.
>
> Mark, Kees, Laura, does it sound good?
Could you have a look at the following changes in check_alloca() before I send
the next version?
If VMAP_STACK is enabled and alloca causes stack depth overflow, I write to
guard page below the thread stack to cause double fault and VMAP_STACK report.
If VMAP_STACK is disabled, I use MIN_STACK_LEFT = 2048, which seems to be enough
for BUG_ON() handling both on x86_32 and x86_64. Unfortunately, I can't
guarantee that it is always enough.
#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
-#define MIN_STACK_LEFT 256
+#define MIN_STACK_LEFT 2048
void __used check_alloca(unsigned long size)
{
unsigned long sp = (unsigned long)&sp;
struct stack_info stack_info = {0};
unsigned long visit_mask = 0;
unsigned long stack_left;
BUG_ON(get_stack_info(&sp, current, &stack_info, &visit_mask));
stack_left = sp - (unsigned long)stack_info.begin;
+
+#ifdef CONFIG_VMAP_STACK
+ /*
+ * If alloca oversteps the thread stack boundary, we touch the guard
+ * page provided by VMAP_STACK to trigger handle_stack_overflow().
+ */
+ if (size >= stack_left)
+ *(stack_info.begin - 1) = 42;
+#else
BUG_ON(stack_left < MIN_STACK_LEFT ||
size >= stack_left - MIN_STACK_LEFT);
+#endif
}
EXPORT_SYMBOL(check_alloca);
#endif
Looking forward to your feedback.
Best regards,
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.
