[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5j+_uxfPPk0ztFW5x9Sua8k=9MmeftiTp76FTnQgJ3Q14g@mail.gmail.com>
Date: Mon, 30 Nov 2015 12:12:27 -0800
From: Kees Cook <keescook@...omium.org>
To: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Cc: Quentin Casasnovas <quentin.casasnovas@...cle.com>
Subject: Re: Re: status: GRKERNSEC_KSTACKOVERFLOW
On Sat, Nov 28, 2015 at 1:19 AM, Quentin Casasnovas
<quentin.casasnovas@...cle.com> wrote:
> On Fri, Nov 27, 2015 at 12:09:31PM -0800, Kees Cook wrote:
>> On Thu, Nov 26, 2015 at 8:39 AM, Quentin Casasnovas
>> <quentin.casasnovas@...cle.com> wrote:
>> > On Thu, Nov 26, 2015 at 12:45:42AM +0100, Quentin Casasnovas wrote:
>> >> On Tue, Nov 24, 2015 at 11:10:09AM -0800, Kees Cook wrote:
>> >>
>> >> [snip/]
>> >>
>> >> It should also be noted that I did not find that the struct thread_info
>> >> (which is stuffed at the end of the stack) was protected in any way either.
>> >> So even if a write/read _below_ the stack could still be trapped if nothing
>> >> is currently mapped there, it looks like deep stack usage could still
>> >> overflow it and go unoticed. Here again, I didn't spend a lot of time on
>> >> this and it might just be that I'm missing something.
>> >>
>> >> In the very unlikely event where I didn't miss anything and the struct
>> >> thread_info can still be overflown and there isn't any guard page, maybe we
>> >> can improve on the current KSTACK_OVERFLOW feature by putting the struct
>> >> thread_info on a different page than the kernel stack, and not vmap() it
>> >> like the rest of the stack pages, but instead map a PROT_NONE page there.
>> >> That would mean the struct thread_info can still be accessed by using its
>> >> lowmem mapping (i.e. legit usage from the kernel) but not by deep kernel
>> >> stack usage. Maybe the cost of adding an extra page per kernel stack is
>> >> too high though.
>> >
>> > As expected I missed some other changes:
>> >
>> > /* Load thread_info address into "reg" */
>> > #define GET_THREAD_INFO(reg) \
>> > - _ASM_MOV PER_CPU_VAR(cpu_current_top_of_stack),reg ; \
>> > - _ASM_SUB $(THREAD_SIZE),reg ;
>> > + _ASM_MOV PER_CPU_VAR(current_tinfo),reg ;
>> >
>> > and
>> >
>> > +DECLARE_PER_CPU(struct thread_info *, current_tinfo);
>> > +
>> > static inline struct thread_info *current_thread_info(void)
>> > {
>> > - return (struct thread_info *)(current_top_of_stack() - THREAD_SIZE);
>> > + return this_cpu_read_stable(current_tinfo);
>> > }
>> >
>> > So no more thread_info on the stack in the default configuration, which
>> > isn't correlated with the KSTACKOVERFLOW config option.
>>
>> Good find! This seems like it should be its own patch, distinct from
>> KSTACKOVERFLOW?
>>
>
> We should probably make KSTACKOVERFLOW depend on moving the thread_info off
> the stack, since otherwise the thread_info could still be hijacked and that
> would counterfeit the KSTACKOVERFLOW purpose.
Right. Either a "select" or "depends" in the Kconfig seems appropriate.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
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.
