linux-kernel - Re: [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting reliable stack traces

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]

Date:	Mon, 4 Apr 2016 17:55:01 +0200
From:	Petr Mladek <pmladek@...e.com>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Jiri Kosina <jikos@...nel.org>, Jessica Yu <jeyu@...hat.com>,
	Miroslav Benes <mbenes@...e.cz>, linux-kernel@...r.kernel.org,
	live-patching@...r.kernel.org, Vojtech Pavlik <vojtech@...e.com>
Subject: Re: [RFC PATCH v1.9 07/14] x86/stacktrace: add function for
 detecting reliable stack traces

On Fri 2016-03-25 14:34:54, Josh Poimboeuf wrote:
> For live patching and possibly other use cases, a stack trace is only
> useful if you can be assured that it's completely reliable.  Add a new
> save_stack_trace_tsk_reliable() function to achieve that.
> 
> Scenarios which indicate that a stack strace may be unreliable:
> 
> - interrupt stacks
> - preemption
> - corrupted stack data
> - newly forked tasks
> - running tasks
> - the user didn't provide a large enough entries array
> 
> Also add a config option so arch-independent code can determine at build
> time whether the function is implemented.
> 
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 3b10518..9c68bfc 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -145,6 +145,42 @@ int print_context_stack_bp(struct thread_info *tinfo,
>  }
>  EXPORT_SYMBOL_GPL(print_context_stack_bp);
>  
> +int print_context_stack_reliable(struct thread_info *tinfo,
> +				 unsigned long *stack, unsigned long *bp,
> +				 const struct stacktrace_ops *ops,
> +				 void *data, unsigned long *end, int *graph)
> +{
> +	struct stack_frame *frame = (struct stack_frame *)*bp;
> +	struct stack_frame *last_frame = frame;

I tried to debug why the patching never finishes as reported by Mirek.
This initialization breaks the whole function, see below.
I would initialize last_frame to NULL or maybe (void *)stack.


> +	unsigned long *ret_addr = &frame->return_address;
> +
> +	if (test_ti_thread_flag(tinfo, TIF_FORK))
> +		return -EINVAL;
> +
> +	while (valid_stack_ptr(tinfo, ret_addr, sizeof(*ret_addr), end)) {
> +		unsigned long addr = *ret_addr;
> +
> +		if (frame <= last_frame || !__kernel_text_address(addr) ||

frame == last_frame in the very rist iteration, so we always return -EINVAL.

Best Regards,
Petr

> +		    in_preempt_schedule_irq(addr))
> +			return -EINVAL;
> +
> +		if (ops->address(data, addr, 1))
> +			return -EINVAL;
> +
> +		print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
> +
> +		last_frame = frame;
> +		frame = frame->next_frame;
> +		ret_addr = &frame->return_address;
> +	}
> +
> +	if (last_frame + 1 != (void *)task_pt_regs(tinfo->task))
> +		return -EINVAL;
> +
> +	*bp = (unsigned long)frame;
> +	return 0;
> +}