Re: [PATCH 02/10] arm64: debug: Ensure debug handlers check triggering exception level

From: Mark Rutland <mark.rutland@arm.com>
To: Will Deacon <will.deacon@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com,
	stable@vger.kernel.org
Subject: Re: [PATCH 02/10] arm64: debug: Ensure debug handlers check triggering exception level
Date: Fri, 1 Mar 2019 13:46:51 +0000	[thread overview]
Message-ID: <20190301134650.GC15517@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20190301132809.24653-3-will.deacon@arm.com>

On Fri, Mar 01, 2019 at 01:28:01PM +0000, Will Deacon wrote:
> Debug exception handlers may be called for exceptions generated both by
> user and kernel code. In many cases, this is checked explicitly, but
> in other cases things either happen to work by happy accident or they
> go slightly wrong. For example, executing 'brk #4' from userspace will
> enter the kprobes code and be ignored, but the instruction will be
> retried forever in userspace instead of delivering a SIGTRAP.
> 
> Fix this issue in the most stable-friendly fashion by simply adding
> explicit checks of the triggering exception level to all of our debug
> exception handlers.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

It might be worth noting in the commit message that this also makes the
functions consistentluy use the DBG_HOOK_* mnemonics, but either way:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/kgdb.c           | 14 ++++++++++----
>  arch/arm64/kernel/probes/kprobes.c |  6 ++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index ce46c4cdf368..691854b77c7f 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -244,27 +244,33 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>  
>  static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
>  {
> +	if (user_mode(regs))
> +		return DBG_HOOK_ERROR;
> +
>  	kgdb_handle_exception(1, SIGTRAP, 0, regs);
> -	return 0;
> +	return DBG_HOOK_HANDLED;
>  }
>  NOKPROBE_SYMBOL(kgdb_brk_fn)
>  
>  static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
>  {
> +	if (user_mode(regs))
> +		return DBG_HOOK_ERROR;
> +
>  	compiled_break = 1;
>  	kgdb_handle_exception(1, SIGTRAP, 0, regs);
>  
> -	return 0;
> +	return DBG_HOOK_HANDLED;
>  }
>  NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
>  
>  static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
>  {
> -	if (!kgdb_single_step)
> +	if (user_mode(regs) || !kgdb_single_step)
>  		return DBG_HOOK_ERROR;
>  
>  	kgdb_handle_exception(1, SIGTRAP, 0, regs);
> -	return 0;
> +	return DBG_HOOK_HANDLED;
>  }
>  NOKPROBE_SYMBOL(kgdb_step_brk_fn);
>  
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index f17afb99890c..7fb6f3aa5ceb 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -450,6 +450,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>  	int retval;
>  
> +	if (user_mode(regs))
> +		return DBG_HOOK_ERROR;
> +
>  	/* return error if this is not our step */
>  	retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
>  
> @@ -466,6 +469,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
>  int __kprobes
>  kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
>  {
> +	if (user_mode(regs))
> +		return DBG_HOOK_ERROR;
> +
>  	kprobe_handler(regs);
>  	return DBG_HOOK_HANDLED;
>  }
> -- 
> 2.11.0
> 