Re: [PATCH 06/10] arm64: debug: Separate debug hooks based on target exception level

From: Mark Rutland <mark.rutland@arm.com>
To: Will Deacon <will.deacon@arm.com>
Cc: catalin.marinas@arm.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 06/10] arm64: debug: Separate debug hooks based on target exception level
Date: Fri, 1 Mar 2019 14:07:49 +0000	[thread overview]
Message-ID: <20190301140749.GF15517@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20190301132809.24653-7-will.deacon@arm.com>

On Fri, Mar 01, 2019 at 01:28:05PM +0000, Will Deacon wrote:
> Mixing kernel and user debug hooks together is highly error-prone as it
> relies on all of the hooks to figure out whether the exception came from
> kernel or user, and then to act accordingly.
> 
> Make our debug hook code a little more robust by maintaining separate
> hook lists for user and kernel, with separate registration functions
> to force callers to be explicit about the exception levels that they
> care about.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>

[...]

>  struct break_hook {
>  	struct list_head node;
> -	u32 esr_val;
> -	u32 esr_mask;
>  	int (*fn)(struct pt_regs *regs, unsigned int esr);
> +	u16 imm;
>  };

It's really nice to see the break_hook data reduced down to the BRK
immediate! Unfortunately, I don't thnk that's sufficient for KASAN;
please see below.

[...]

>  static int call_break_hook(struct pt_regs *regs, unsigned int esr)
>  {
>  	struct break_hook *hook;
> +	struct list_head *list;
>  	int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
>  
> +	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
> +
>  	rcu_read_lock();
> -	list_for_each_entry_rcu(hook, &break_hook, node)
> -		if ((esr & hook->esr_mask) == hook->esr_val)
> +	list_for_each_entry_rcu(hook, list, node)
> +		if ((esr & BRK64_ESR_MASK) == hook->imm)
>  			fn = hook->fn;

Could we please fix up the existing coding style bug by placing braces
around the loop body?

Coding style says we should, since it's more than a single simple
statement, and it would better match what we do in call_step_hook().

[...]

> -#define KASAN_ESR_VAL (0xf2000000 | KASAN_BRK_IMM)
> -#define KASAN_ESR_MASK 0xffffff00
> -
>  static struct break_hook kasan_break_hook = {
> -	.esr_val = KASAN_ESR_VAL,
> -	.esr_mask = KASAN_ESR_MASK,
>  	.fn = kasan_handler,
> +	.imm = KASAN_BRK_IMM,
>  };
>  #endif
>  
> @@ -1030,7 +1025,7 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
>  		struct pt_regs *regs)
>  {
>  #ifdef CONFIG_KASAN_SW_TAGS
> -	if ((esr & KASAN_ESR_MASK) == KASAN_ESR_VAL)
> +	if ((esr & BRK64_ESR_MASK) == KASAN_BRK_IMM)

According to <asm/brk-imm.h>, KASAN BRKs can use the whole 0x900-0x9ff
range of immediates, and these changes mean that we'll only match 0x900.

We probably need an imm_mask field on struct break_hook to cater for
that.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel