Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 14 Jul 2017 13:51:24 -0700
From: Laura Abbott <labbott@...hat.com>
To: Mark Rutland <mark.rutland@....com>
Cc: Kees Cook <keescook@...omium.org>, Alex Popov <alex.popov@...ux.com>,
 kernel-hardening@...ts.openwall.com,
 Ard Biesheuvel <ard.biesheuvel@...aro.org>
Subject: Re: [RFC][PATCH 2/2] arm64: Clear the stack

On 07/11/2017 12:51 PM, Mark Rutland wrote:
> Hi Laura,
> 
> On Mon, Jul 10, 2017 at 03:04:43PM -0700, Laura Abbott wrote:
>> Implementation of stackleak based heavily on the x86 version
> 
> Nice!
> 
>> Signed-off-by: Laura Abbott <labbott@...hat.com>
>> ---
>> The biggest points that need review here:
>> - Is the extra offsetting (subtracting/adding from the stack) correct?
> 
> I think it's a little off; more on that below.
> 
>> - Where else do we need to clear the stack?
> 
> I guess we might need to clear (all of the remainder of) the stack after
> invoking EFI runtime services -- those can run in task context, might
> leave sensitive values on the stack, and they're uninstrumented. The
> same would apply for x86.
> 
> I think we can ignore garbage left on the stack by idle/hotplug, since
> that happens in the idle thread, so we shouldn't be doing uaccess
> transfers on those stacks.
> 
>> - The assembly can almost certainly be optimized. I tried to keep the
>>   behavior of the x86 'repe scasq' and the like where possible. I'm also a
>>   terrible register allocator.
> 
> I tried to steer clear of code golf here, but I didn't entirely manage.
> 
> I also don't know x86 asm, so it's very possible I've just managed to
> confuse myself.
> 

I know enough x86 asm to confuse myself as you can see below

>> ---
>>  arch/arm64/Kconfig                    |  1 +
>>  arch/arm64/include/asm/processor.h    |  3 ++
>>  arch/arm64/kernel/asm-offsets.c       |  3 ++
>>  arch/arm64/kernel/entry.S             | 92 +++++++++++++++++++++++++++++++++++
>>  arch/arm64/kernel/process.c           | 18 +++++++
>>  drivers/firmware/efi/libstub/Makefile |  3 +-
>>  scripts/Makefile.gcc-plugins          |  5 +-
>>  7 files changed, 123 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 8addb85..0b65bfc 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -17,6 +17,7 @@ config ARM64
>>  	select ARCH_HAS_KCOV
>>  	select ARCH_HAS_SET_MEMORY
>>  	select ARCH_HAS_SG_CHAIN
>> +	select ARCH_HAS_STACKLEAK
>>  	select ARCH_HAS_STRICT_KERNEL_RWX
>>  	select ARCH_HAS_STRICT_MODULE_RWX
>>  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> index 64c9e78..76f2738 100644
>> --- a/arch/arm64/include/asm/processor.h
>> +++ b/arch/arm64/include/asm/processor.h
>> @@ -88,6 +88,9 @@ struct thread_struct {
>>  	unsigned long		fault_address;	/* fault info */
>>  	unsigned long		fault_code;	/* ESR_EL1 value */
>>  	struct debug_info	debug;		/* debugging */
>> +#ifdef CONFIG_STACKLEAK
>> +	unsigned long           lowest_stack;
>> +#endif
>>  };
>>  
>>  #ifdef CONFIG_COMPAT
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index b3bb7ef..e0a5ae2 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -43,6 +43,9 @@ int main(void)
>>    DEFINE(TSK_TI_TTBR0,		offsetof(struct task_struct, thread_info.ttbr0));
>>  #endif
>>    DEFINE(TSK_STACK,		offsetof(struct task_struct, stack));
>> +#ifdef CONFIG_STACKLEAK
>> +  DEFINE(TSK_TI_LOWEST_STACK,	offsetof(struct task_struct, thread.lowest_stack));
>> +#endif
>>    BLANK();
>>    DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
>>    BLANK();
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index b738880..e573633 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -744,6 +744,7 @@ ENDPROC(cpu_switch_to)
>>   */
>>  ret_fast_syscall:
>>  	disable_irq				// disable interrupts
>> +	bl	erase_kstack
>>  	str	x0, [sp, #S_X0]			// returned x0
>>  	ldr	x1, [tsk, #TSK_TI_FLAGS]	// re-check for syscall tracing
>>  	and	x2, x1, #_TIF_SYSCALL_WORK
>> @@ -772,6 +773,7 @@ work_pending:
>>   */
>>  ret_to_user:
>>  	disable_irq				// disable interrupts
>> +	bl	erase_kstack
>>  	ldr	x1, [tsk, #TSK_TI_FLAGS]
>>  	and	x2, x1, #_TIF_WORK_MASK
>>  	cbnz	x2, work_pending
>> @@ -865,3 +867,93 @@ ENTRY(sys_rt_sigreturn_wrapper)
>>  	mov	x0, sp
>>  	b	sys_rt_sigreturn
>>  ENDPROC(sys_rt_sigreturn_wrapper)
>> +
>> +#ifdef CONFIG_STACKLEAK
>> +ENTRY(erase_kstack)
>> +	/* save x0 for the fast path */
>> +	mov	x10, x0
>> +
>> +	get_thread_info	x0
>> +	ldr	x1, [x0, #TSK_TI_LOWEST_STACK]
>> +
>> +	/* get the number of bytes to check for lowest stack */
>> +	mov	x3, x1
>> +	and	x3, x3, #THREAD_SIZE - 1
>> +	lsr	x3, x3, #3
>> +
>> +	/* now generate the start of the stack */
>> +	mov	x4, sp
>> +	movn	x2, #THREAD_SIZE - 1
>> +	and	x1, x4, x2
>> +
>> +	mov	x2, #-0xBEEF	/* stack poison */
>> +
>> +	cmp     x3, #0
>> +	b.eq    4f /* Found nothing, go poison */
>> +
>> +1:
>> +	ldr     x4, [x1, x3, lsl #3]
>> +	cmp     x4, x2  /* did we find the poison? */
>> +	b.eq    2f /* yes we did, go check it */
>> +
>> +	sub     x3, x3, #1
>> +	cmp     x3, #0
>> +	b.eq    4f /* Found nothing, go poison */
>> +	b       1b /* loop again */
>> +
>> +2:
> 
> IIUC, at this point, x1 was the start (lowest addr) of the stack, and x3
> was the quadword index of the first poison on that stack.
> 
> The x86 asm implicitly held that [x1, x3, lsl #3] address in RDI, but we
> don't have a copy...
> 
>> +	cmp     x3, #16
>> +	b.ls    4f /* Go poison if there are less than 16 things left? */
>> +
>> +	mov     x3, #16
> 
> ... and here we blat the index without saving it, meaning we always jump
> to close to the start of the stack, which I don't think was intentional.
> 

Urgh. You are right. I had an index register when I first
started out and then a different bug caused me to erroneously
remove it.

> So then we fall though to the below...
> 
>> +3:
>> +	ldr     x4, [x1, x3, lsl #3]
>> +	cmp     x4, x2  /* did we find the poison? */
>> +	b.ne    1b /* nope we have to check deeper */
>> +
>> +	sub     x3, x3, #1
>> +	cmp     x3, #0
>> +	b.eq    4f /* Found nothing, go poison */
>> +	b       3b /* loop again */
> 
> ... where we either:
> 
> * find 16 contiguous poison entries, leaving x3 == 0, or:
> 
> * we immediately find a non-poison value, and jump back to 1b. If there
>   are 16 non-poison values, we're left with x3 == 0, otherwise we bail
>   out and jump to 4f with x3 in the interval [0,15].
> 
> .... or I've completely confused myself, as suggested above.
> 
> We might not have x86's fancy string instructions, but we could do
> something equally impenetrable:
> 
> 	mov	x5, #0
> 1:
> 	cbz	x3, 4f
> 	ldr	x4, [x1, x3, lsl #3]
> 	cmp	x4, x2
> 	csinc	x5, xzr, x5, ne
> 	tbz	x5, #4, 4f		// found 16 poisons?
> 	sub	x3, x3, #1
> 	b	1b
> 
> That doesn't stop 16 slots early, so it can return any value of x3 all
> the way down to zero.
> 

Seems to work for my testing.

>> +
>> +	/* The poison function */
>> +4:
>> +	/* Generate the address we found */
>> +	add     x5, x1, x3, lsl #3
>> +	orr     x5, x5, #16
> 
> Have you figured out what the forced 16 byte offset is for?
> 
> I've not managed to figure out why it's there, and it looks like
> Alexander couldn't either, judging by his comments in the x86 asm.
>

>From get_wchan in arch/x86/kernel/process.c, it might be be to
account for the start of the frame correctly? I don't have a
definitive answer though and plan on just removing this for the
next version.
 
>> >> +	mov	x4, sp
>> +	/* subtrace the current pointer */
>> +	sub     x8, x4, x5
>> +
>> +	/* subtract one more so we don't touch the current sp */
>> +	sub	x8, x8, #1
>> +
>> +	/* sanity check */
>> +	cmp     x8, #THREAD_SIZE
>> +	b.lo    5f
>> +999:
>> +	b       999b
>> +
>> +5:
>> +	lsr     x8, x8, #3
>> +	mov     x3, x8
>> +6:
>> +	cmp     x3, #0
>> +	b.eq    7f
>> +
>> +	str     x2, [x1, x3, lsl #3]
>> +	sub     x3, x3, #1
>> +	b	6b
>> +
>> +	/* Reset the lowest stack to the top of the stack */
>> +7:
>> +	ldr	x1, [x0, TSK_STACK]
>> +	add	x1, x1, #THREAD_SIZE
>> +	sub	x1, x1, #256
>> +	str	x1, [x0, #TSK_TI_LOWEST_STACK]
> 
> I take it this is the offsetting you were querying?
> 
> I don't think it's quite right. Our stack looks like:
> 
> +---+ <- task_stack_page(p) + THREAD_SIZE
> |   |
> +---+ <- task_stack_page(p) + THREAD_START_SP
> |   |
> |   |
> +---+ <- task_pt_regs(p)
> |   |
> |   |
> |   |
> ~~~~~
> 
> ~~~~~
> |   |
> |   |
> |   |
> +---+ <- task_stack_page(p)
> 
> At the point we return to userspace, sp == task_pt_regs(p).
> 
> Judging by a generated asm-offsets.h, sizeof(struct_pt_regs) is 304
> bytes currently. THREAD_SIZE - THREAD_START_SP == 16.
> 
> We probably want to give that 16 a mnemonic (e.g FRAME_PADDING), and
> have something like:
> 
> 	ldr     x1, [x0, TSK_STACK]
> 	add	x1, x1, #THREAD_SIZE
> 	sub	x1, x1, #(S_FRAME_SIZE + FRAME_PADDING)
> 	str	x1, [x0, #TSK_TI_LOWEST_STACK]
> 

Yes, that looks cleaner. I suspect that even though we weren't
subtracting enough, it still worked because the lowest stack
would get overwritten in track_stack later.

>> +
>> +	mov	x0, x10
>> +	ret
>> +ENDPROC(erase_kstack)
>> +#endif
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 659ae80..1b6cca2 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -293,6 +293,12 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
>>  	p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
>>  	p->thread.cpu_context.sp = (unsigned long)childregs;
>>  
>> +#ifdef CONFIG_STACKLEAK
>> +	p->thread.lowest_stack = (unsigned long)task_stack_page(p) +
>> +					2 * sizeof(unsigned long);
>> +#endif
> 
> I see this follows the x86 logic, but I don't see why it's necessary to
> offset this end of the stack.
> 
> Do you have an idea as to what this is for?
> 

Again, no and I think I'll again remove it.

>> +
>> +
>>  	ptrace_hw_copy_thread(p);
>>  
>>  	return 0;
>> @@ -417,3 +423,15 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>>  	else
>>  		return randomize_page(mm->brk, SZ_1G);
>>  }
>> +
>> +#ifdef CONFIG_STACKLEAK
>> +void __used check_alloca(unsigned long size)
>> +{
>> +	unsigned long sp = (unsigned long)&sp, stack_left;
> 
> Nit: please use current_stack_pointer.
> 

Ack. This should also be cleaned up in the the track_stack
function as well.

> Thanks,
> Mark.
> 

Thanks,
Laura

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.