Re: [PATCH] arm64: Add copy_mc_to_kernel() that recovers from synchronous external aborts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] arm64: copy_mc_to_kernel macro bypasses MTE, KASAN, and cache page copy
- [High] arm64: extable: insn_may_access_user() livelocks on EX_TYPE_KACCESS_MC
- [High] arm64: fault: do_sea() apei_claim_sea() sends SIGBUS on recovered fault
--

commit 5a611f7bbfb0b0b0a16c229590b9cf45b688ab1a
Author: Adi Nikumbh <nikumbh.adi@gmail.com>

arm64: Add copy_mc_to_kernel() that recovers from synchronous external aborts

This commit adds copy_mc_to_kernel() and the EX_TYPE_KACCESS_MC exception
table type on arm64 so kernel loads from poisoned memory can recover from
synchronous external aborts instead of oopsing. In do_sea(), EL1 data aborts
matching an external abort fault status on an EX_TYPE_KACCESS_MC instruction
branch to the fixup and call apei_claim_sea().

> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 9f5bd9c69c249..19596fa3d6706 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -474,6 +474,10 @@ extern long strncpy_from_user(char *dest, const char __user *src, long count);
>  
>  extern __must_check long strnlen_user(const char __user *str, long n);
>  
> +unsigned long __must_check
> +copy_mc_to_kernel(void *to, const void *from, size_t size);
> +#define copy_mc_to_kernel copy_mc_to_kernel

[Severity: Critical]
Does defining the copy_mc_to_kernel macro here without arm64-specific
copy_mc_highpage() and copy_mc_user_highpage() implementations bypass MTE tag
handling, KASAN tag resets, and D-cache maintenance on page copies?

In include/linux/highmem.h, defining copy_mc_to_kernel switches
copy_mc_user_highpage() and copy_mc_highpage() away from calling arm64's
copy_user_highpage() and copy_highpage() in arch/arm64/mm/copypage.c:

include/linux/highmem.h:
#ifdef copy_mc_to_kernel
static inline int copy_mc_user_highpage(struct page *to, struct page *from,
					unsigned long vaddr, struct vm_area_struct *vma)
{
	unsigned long ret;
	char *vfrom, *vto;

	vfrom = kmap_local_page(from);
	vto = kmap_local_page(to);
	ret = copy_mc_to_kernel(vto, vfrom, PAGE_SIZE);
	...
}

Callers on the Copy-on-Write and page migration paths (such as
__wp_page_copy_user(), do_cow_fault(), copy_present_page(),
copy_user_large_folio(), and folio_mc_copy()) now invoke
copy_mc_to_kernel() directly instead of arch/arm64/mm/copypage.c:

arch/arm64/mm/copypage.c:copy_highpage() {
	...
	copy_page(kto, kfrom);

	if (kasan_hw_tags_enabled())
		page_kasan_tag_reset(to);

	if (!system_supports_mte())
		return;

	if (folio_test_hugetlb(src) &&
	    folio_test_hugetlb_mte_tagged(src)) {
		if (!folio_try_hugetlb_mte_tagging(dst))
			return;
		mte_copy_page_tags(kto, kfrom);
		folio_set_hugetlb_mte_tagged(dst);
	} else if (page_mte_tagged(from)) {
		try_page_mte_tagging(to);

		mte_copy_page_tags(kto, kfrom);
		set_page_mte_tagged(to);
	}
}

arch/arm64/mm/copypage.c:copy_user_highpage() {
	copy_highpage(to, from);
	flush_dcache_page(to);
}

Skipping copy_user_highpage() and copy_highpage() causes four regressions:

1. MTE tags are not copied via mte_copy_page_tags() and PG_mte_tagged is left
   unset on the destination page. When set_pte_at() later calls
   __sync_cache_and_tags() -> mte_sync_tags() in arch/arm64/kernel/mte.c, it
   sees PG_mte_tagged unset and zeroes all tags on the destination page via
   mte_clear_page_tags(), corrupting userspace MTE tags across CoW faults and
   migrations.

2. page_kasan_tag_reset(to) is skipped when kasan_hw_tags_enabled() is true.

3. flush_dcache_page(to) is skipped on user page copies, leaving
   PG_dcache_clean stale when executable pages are copied.

4. Every CoW fault and page migration drops from copy_page() (which uses
   FEAT_MOPS or 128-byte ldp/stnp loops) to the 8-byte ldr/str loop in
   __memcpy_mc().

> diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
> index 299a244f68e60..22b0356664bea 100644
> --- a/arch/arm64/mm/extable.c
> +++ b/arch/arm64/mm/extable.c
> @@ -105,7 +105,22 @@ bool fixup_exception(struct pt_regs *regs, unsigned long esr)
>  		return ex_handler_uaccess_cpy(ex, regs, esr);
>  	case EX_TYPE_LOAD_UNALIGNED_ZEROPAD:
>  		return ex_handler_load_unaligned_zeropad(ex, regs);
> +	case EX_TYPE_KACCESS_MC:
> +		/* Only a synchronous external abort is fixed up, by fixup_exception_mc() */
> +		return false;
>  	}

[Severity: High]
Should insn_may_access_user() in arch/arm64/mm/extable.c also return false for
EX_TYPE_KACCESS_MC?

Currently, insn_may_access_user() returns true in its default branch for all
exception table types other than EX_TYPE_UACCESS_CPY:

arch/arm64/mm/extable.c:insn_may_access_user() {
	const struct exception_table_entry *ex = search_exception_tables(addr);

	if (!ex)
		return false;

	switch (ex->type) {
	case EX_TYPE_UACCESS_CPY:
		return cpy_faulted_on_uaccess(ex, esr);
	default:
		return true;
	}
}

Unlike copy_from_kernel_nofault() (which runs with pagefault_disable() so
do_page_fault() immediately takes the no_context path into fixup_exception()),
copy_mc_to_kernel() runs with page faults enabled.

If copy_mc_to_kernel() -> __memcpy_mc() is passed a TTBR0 address (such as a
user pointer or NULL with CONFIG_ARM64_SW_TTBR0_PAN) in process context, the
privileged ldr/ldrb in __memcpy_mc() triggers a permission fault with PAN
active:

do_page_fault()
  if (is_ttbr0_addr(addr) && is_el1_permission_fault(addr, esr, regs)) {
      ...
      if (!insn_may_access_user(regs->pc, esr))
          die_kernel_fault("access to user memory outside uaccess routines",
                           addr, esr, regs);
  }

Because insn_may_access_user() returns true for EX_TYPE_KACCESS_MC,
do_page_fault() skips die_kernel_fault(), takes current->mm's VMA / mmap lock,
and calls handle_mm_fault(). If the address is mapped in a readable user VMA,
handle_mm_fault() succeeds and do_page_fault() returns 0 without advancing
regs->pc or clearing PAN, causing the eret to re-execute the same ldr/ldrb
instruction in an infinite page-fault loop.

> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index b22736853fcd8..f65304f29f683 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
[ ... ]
> @@ -889,6 +909,19 @@ static int do_sea(unsigned long far, unsigned long esr, struct pt_regs *regs)
>  		return 0;
>  	}
>  
> +	if (!user_mode(regs) && is_el1_recoverable_sea(esr) &&
> +	    fixup_exception_mc(regs)) {
> +		/*
> +		 * copy_mc_to_kernel() read memory with an uncorrectable error.
> +		 * It returns the bytes it could not copy and its caller handles
> +		 * the failure, so the kernel does not need to die. Firmware-first
> +		 * systems still get to record the error.
> +		 */
> +		apei_claim_sea(regs);
> +		add_taint(TAINT_MACHINE_CHECK, LOCKDEP_STILL_OK);
> +		return 0;
> +	}

[Severity: High]
Won't calling apei_claim_sea(regs) here cause ACPI APEI GHES to send SIGBUS to
current even though the kernel already recovered from the fault via
fixup_exception_mc()?

When apei_claim_sea() runs ghes_notify_sea() -> ghes_proc_in_irq() ->
ghes_do_proc() in drivers/acpi/apei/ghes.c, sync is true because
is_hest_sync_notify(ghes) is true for ACPI_HEST_NOTIFY_SEA:

1. If no memory failure work is queued (!queued, for example when
   CONFIG_ACPI_APEI_MEMORY_FAILURE=n or the CPER record lacks a valid physical
   address), ghes_do_proc() unconditionally forces SIGBUS on current:

   drivers/acpi/apei/ghes.c:ghes_do_proc() {
       ...
       if (sync && !queued) {
           dev_err(ghes->dev,
               HW_ERR GHES_PFX "%s:%d: synchronous unrecoverable error (SIGBUS)\n",
               current->comm, task_pid_nr(current));
           force_sig(SIGBUS);
       }
   }

2. If memory failure work is queued, ghes_do_memory_failure() sets
   flags = MF_ACTION_REQUIRED because sync is true, and when current->mm is
   non-NULL it queues memory_failure_cb() as TWA_RESUME task_work on current:

   drivers/acpi/apei/ghes.c:memory_failure_cb() {
       ...
       ret = memory_failure(twcb->pfn, twcb->flags);
       gen_pool_free(ghes_estatus_pool, (unsigned long)twcb, sizeof(*twcb));

       if (!ret || ret == -EHWPOISON || ret == -EOPNOTSUPP)
           return;

       pr_err("%#llx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
               twcb->pfn, current->comm, task_pid_nr(current));
       force_sig(SIGBUS);
   }

When copy_mc_to_kernel() is called on behalf of a kernel subsystem (such as
pmem, dax, dm-writecache, or page migration) and recovers from the SEA,
memory_failure(pfn, MF_ACTION_REQUIRED) will either signal current or return
an error like -ENXIO, -EBUSY, or -EFAULT (when the PFN is not mapped in
current->mm), causing memory_failure_cb() to kill the calling user task with
SIGBUS instead of letting the caller handle the failed copy.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028045028.72644-1-nikumbh.adi@gmail.com?part=1