Re: [PATCH v2 06/11] arm64: debug: split hardware breakpoint exeception entry

[Will Deacon <will@kernel.org>]

nit: typo in $subject ("exeception").

On Mon, May 12, 2025 at 06:43:21PM +0100, Ada Couprie Diaz wrote:
> Currently all debug exceptions share common entry code and are routed
> to `do_debug_exception()`, which calls dynamically-registered
> handlers for each specific debug exception. This is unfortunate as
> different debug exceptions have different entry handling requirements,
> and it would be better to handle these distinct requirements earlier.
> 
> Hardware breakpoints exceptions are generated by the hardware after user
> configuration. As such, they can be exploited when training branch
> predictors outisde of the userspace VA range: they still need to call

"outisde"

> `arm64_apply_bp_hardening()` if needed to mitigate against this attack.
> Move the call to `arm64_apply_bp_hardening()` to `entry-common.c` as
> it is needed for exceptions coming from EL0 only.
> 
> However, they do not need to handle the Cortex-A76 erratum #1463225 as
> it only applies to single stepping exceptions.
> It does not set an address in FAR_EL1 either, only the hardware
> watchpoint does.
>
> Split the hardware breakpoint exception entry, adjust
> the function signature, and handling of the Cortex-A76 erratum to fit
> the behaviour of the exception.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/exception.h |  1 +
>  arch/arm64/kernel/entry-common.c   | 28 ++++++++++++++++++++++++++++
>  arch/arm64/kernel/hw_breakpoint.c  | 15 +++++++++++----
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index d48fc16584cd..c593fe639697 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -61,6 +61,7 @@ void do_el0_gcs(struct pt_regs *regs, unsigned long esr);
>  void do_el1_gcs(struct pt_regs *regs, unsigned long esr);
>  void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
>  			struct pt_regs *regs);
> +void do_breakpoint(unsigned long esr, struct pt_regs *regs);
>  void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sve_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sme_acc(unsigned long esr, struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 92d78b329e67..6ff52fc94da7 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -503,6 +503,15 @@ static void noinstr el1_mops(struct pt_regs *regs, unsigned long esr)
>  	exit_to_kernel_mode(regs);
>  }
>  
> +static void noinstr el1_breakpt(struct pt_regs *regs, unsigned long esr)
> +{
> +	arm64_enter_el1_dbg(regs);
> +	debug_exception_enter(regs);
> +	do_breakpoint(esr, regs);
> +	debug_exception_exit(regs);
> +	arm64_exit_el1_dbg(regs);
> +}
> +
>  static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
>  {
>  	unsigned long far = read_sysreg(far_el1);
> @@ -552,6 +561,8 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
>  		el1_mops(regs, esr);
>  		break;
>  	case ESR_ELx_EC_BREAKPT_CUR:
> +		el1_breakpt(regs, esr);
> +		break;
>  	case ESR_ELx_EC_SOFTSTP_CUR:
>  	case ESR_ELx_EC_WATCHPT_CUR:
>  	case ESR_ELx_EC_BRK64:
> @@ -746,6 +757,19 @@ static void noinstr el0_inv(struct pt_regs *regs, unsigned long esr)
>  	exit_to_user_mode(regs);
>  }
>  
> +static void noinstr el0_breakpt(struct pt_regs *regs, unsigned long esr)
> +{
> +	if (!is_ttbr0_addr(regs->pc))
> +		arm64_apply_bp_hardening();

I think this is a change in behaviour, as arm64_apply_bp_hardening() is
now called before enter_from_user_mode() and debug_exception_enter().
Is that safe and intentional?

Will