linux-kernel - Re: [PATCH 2/2] arm64: Clear the stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<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