Re: [PATCH 2/4] x86/unwind: ensure stack grows down

[Ingo Molnar <mingo@kernel.org>]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Add a sanity check to ensure the stack only grows down, and print a
> warning if the check fails.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/kernel/unwind_frame.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index 9be9a8f..0d8d2f2 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -24,6 +24,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
>  }
>  EXPORT_SYMBOL_GPL(unwind_get_return_address);
>  
> +static size_t regs_size(struct pt_regs *regs)
> +{
> +	/* x86_32 regs from kernel mode are two words shorter */
> +	if (IS_ENABLED(CONFIG_X86_32) && !user_mode(regs))
> +		return sizeof(*regs) - (2*sizeof(long));

Unnecessary parentheses.

> +
> +	return sizeof(*regs);
> +}
> +
>  static bool is_last_task_frame(struct unwind_state *state)
>  {
>  	unsigned long bp = (unsigned long)state->bp;
> @@ -71,6 +80,7 @@ bool unwind_next_frame(struct unwind_state *state)
>  	struct pt_regs *regs;
>  	unsigned long *next_bp, *next_frame;
>  	size_t next_len;
> +	enum stack_type prev_type = state->stack_info.type;
>  
>  	if (unwind_done(state))
>  		return false;
> @@ -134,6 +144,18 @@ bool unwind_next_frame(struct unwind_state *state)
>  		goto bad_address;
>  	}
>  
> +	/* make sure it only unwinds up and doesn't overlap the last frame */

Please capitalize comments - and if the comment refers to a control block please 
also add ':', i.e. something like:

	/* Make sure it only unwinds up and doesn't overlap the last frame: */

This also applies to other places in this patch series.

> +	if (state->stack_info.type == prev_type) {
> +		if (state->regs &&
> +		    (void *)next_frame < (void *)state->regs +
> +					 regs_size(state->regs))

No line breaks that make the code worse please.

> +			goto bad_address;
> +
> +		if (state->bp &&
> +		    (void *)next_frame < (void *)state->bp + FRAME_HEADER_SIZE)

Ditto.

> +			goto bad_address;
> +	}
> +
>  	/* move to the next frame */
>  	if (regs) {
>  		state->regs = regs;
> @@ -146,10 +168,16 @@ bool unwind_next_frame(struct unwind_state *state)
>  	return true;
>  
>  bad_address:
> -	printk_deferred_once(KERN_WARNING
> -		"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
> -		state->bp, state->task->comm,
> -		state->task->pid, next_bp);
> +	if (state->regs)
> +		printk_deferred_once(KERN_WARNING
> +			"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
> +			state->regs, state->task->comm,
> +			state->task->pid, next_frame);
> +	else
> +		printk_deferred_once(KERN_WARNING
> +			"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
> +			state->bp, state->task->comm,
> +			state->task->pid, next_frame);

Multi-line statements should have curly braces.

Thanks,

	Ingo