Re: [PATCH RFC 2/3] arm64: Allow synchronous abort fixups for guarded MMIO reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] arm64: extable: fixup_exception() and insn_may_access_user()
  match non-SEA faults
- [Low] arm64: extable: missing __DEFINE_ASM_GPR_NUMS in
  _ASM_EXTABLE_MMIO_READ_ERR_ZERO()
--

commit 1c63c0d5426ef0604198833160287cf9aa8cf068
Author: Matt Thompson <matt@boondocker.io>

arm64: Allow synchronous abort fixups for guarded MMIO reads

This commit adds the EX_TYPE_MMIO_READ_ERR_ZERO exception table type and
hooks it into do_sea() and fixup_exception() to fix up synchronous
external data aborts on guarded MMIO read instructions.

> diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h
> index d67e2fdd1aee5..ed80aec162798 100644
> --- a/arch/arm64/include/asm/asm-extable.h
> +++ b/arch/arm64/include/asm/asm-extable.h
[ ... ]
> @@ -123,6 +124,14 @@
>  #define _ASM_EXTABLE_KACCESS(insn, fixup)				\
>  	_ASM_EXTABLE_KACCESS_ERR_ZERO(insn, fixup, wzr, wzr)
>  
> +#define _ASM_EXTABLE_MMIO_READ_ERR_ZERO(insn, fixup, err, zero)		\
> +	__ASM_EXTABLE_RAW(insn, fixup,					\
> +			  __stringify(EX_TYPE_MMIO_READ_ERR_ZERO),	\
> +			  "((.L__gpr_num_" err " << "			\
> +			  __stringify(EX_DATA_REG_ERR_SHIFT) ") | "	\
> +			  "(.L__gpr_num_" zero " << "			\
> +			  __stringify(EX_DATA_REG_ZERO_SHIFT) "))")
> +

[Severity: Low]
Should _ASM_EXTABLE_MMIO_READ_ERR_ZERO() emit __DEFINE_ASM_GPR_NUMS and
use #insn, #fixup and EX_DATA_REG() like _ASM_EXTABLE_KACCESS_ERR_ZERO()?

Without __DEFINE_ASM_GPR_NUMS, any C file using this macro fails to
assemble unless it defines the .L__gpr_num_* symbols itself, and callers
have to pass pre-quoted string literals for the labels and register
names.

> diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
> index 299a244f68e60..c9b59449fbb24 100644
> --- a/arch/arm64/mm/extable.c
> +++ b/arch/arm64/mm/extable.c
> @@ -100,6 +100,7 @@ bool fixup_exception(struct pt_regs *regs, unsigned long esr)
>  		return ex_handler_bpf(ex, regs);
>  	case EX_TYPE_UACCESS_ERR_ZERO:
>  	case EX_TYPE_KACCESS_ERR_ZERO:
> +	case EX_TYPE_MMIO_READ_ERR_ZERO:
>  		return ex_handler_uaccess_err_zero(ex, regs);
>  	case EX_TYPE_UACCESS_CPY:
>  		return ex_handler_uaccess_cpy(ex, regs, esr);
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index b22736853fcd8..148f7c98ab444 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -877,8 +877,19 @@ static int do_bad(unsigned long far, unsigned long esr, struct pt_regs *regs)
>  static int do_sea(unsigned long far, unsigned long esr, struct pt_regs *regs)
>  {
>  	const struct fault_info *inf;
> +	const struct exception_table_entry *ex;
>  	unsigned long siaddr;
>  
> +	/* Only a synchronous data abort on an explicitly guarded MMIO read. */
> +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_DABT_CUR &&
> +	    (esr & ESR_ELx_FSC) == ESR_ELx_FSC_EXTABT &&
> +	    !(esr & ESR_ELx_WNR)) {
> +		ex = search_exception_tables(instruction_pointer(regs));
> +		if (ex && ex->type == EX_TYPE_MMIO_READ_ERR_ZERO &&
> +		    fixup_exception(regs, esr))
> +			return 0;
> +	}

[Severity: Medium]
Can fixup_exception() and insn_may_access_user() still match
EX_TYPE_MMIO_READ_ERR_ZERO on non-SEA faults?

The ESR check is in do_sea(), but fixup_exception() handles
EX_TYPE_MMIO_READ_ERR_ZERO unconditionally and is also called from
__do_kernel_fault() and do_gpf().  A translation or alignment fault on a
guarded MMIO read (such as a NULL or unmapped iomem pointer) will be
silently fixed up to -EFAULT instead of oopsing.

In addition, insn_may_access_user() returns true in its default case for
EX_TYPE_MMIO_READ_ERR_ZERO:

arch/arm64/mm/extable.c:insn_may_access_user() {
	...
	switch (ex->type) {
	case EX_TYPE_UACCESS_CPY:
		return cpy_faulted_on_uaccess(ex, esr);
	default:
		return true;
	}
}

If a guarded MMIO read in process context faults on a mapped readable
TTBR0 address while PAN is active, do_page_fault() treats it as a valid
uaccess instead of calling die_kernel_fault(), and handle_mm_fault()
returns 0 without advancing regs->pc, causing an infinite fault loop.

do_sea() also calls search_exception_tables() before calling
fixup_exception(), which searches the exception tables a second time.
Checking esr in a dedicated handler in arch/arm64/mm/extable.c and
returning false for EX_TYPE_MMIO_READ_ERR_ZERO in insn_may_access_user()
would restrict the fixup to synchronous external read aborts across all
callers and avoid the duplicate lookup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-rfc-b4-v1-0-b91dcb502250@boondocker.io?part=2