Re: [PATCH] arm64: errata: Add NXP iMX8QM workaround for A53 cache coherency issue

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional upgrade of local TLB flushes to global broadcast (`vmalle1is`), resulting in unsynchronized broadcast DVM messages.
- [High] The workaround for user-space IC IVAU silently swallows faults for invalid or kernel addresses.
--

--- Patch [1]: [PATCH] arm64: errata: Add NXP iMX8QM workaround for A53 cache coherency issue ---
commit 2862fd74023d9e9e12c6a2745e844dc84b6cf1f7
Author: Peng Fan <[email protected]>

arm64: errata: Add NXP iMX8QM workaround for A53 cache coherency issue

According to NXP errata document IMX8_1N94W, the i.MX8QuadMax SoC suffers
from a cache coherency issue (ERR050104). The workaround includes downgrading
targeted TLBI operations to broadcast-all variants, upgrading IC IVAU to
IC IALLUIS, and disabling KVM.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
> index 14a78ac0f8009..87843e602127e 100644
> --- a/arch/arm64/include/asm/tlbflush.h
> +++ b/arch/arm64/include/asm/tlbflush.h
[ ... ]
> @@ -580,23 +584,27 @@ static __always_inline void __do_flush_tlb_range(struct vm_area_struct *vma,
>  
>  	asid = ASID(mm);
>  
> -	switch (flags & (TLBF_NOWALKCACHE | TLBF_NOBROADCAST)) {
> -	case TLBF_NONE:
> -		__flush_s1_tlb_range_op(vae1is, start, pages, stride,
> -					asid, tlb_level);
> -		break;
[ ... ]
> +	if (alternative_has_cap_unlikely(ARM64_WORKAROUND_NXP_ERR050104)) {
> +		__tlbi(vmalle1is);

[Severity: High]
Does this code inadvertently break the synchronization for local TLB flushes?
When __do_flush_tlb_range() is called with the TLBF_NOBROADCAST flag (such
as from flush_tlb_fix_spurious_fault()), this block unconditionally upgrades
the local flush to a global broadcast (__tlbi(vmalle1is)).

However, later in the same function, if TLBF_NOBROADCAST is set, the code
synchronizes the operation using dsb(nsh) instead of dsb(ish). Emitting an
inner-shareable broadcast TLB instruction but only waiting for local
completion violates the hardware contract and can overwhelm the system
interconnect with DVM messages that are never waited for.

Should local flushes be upgraded to the local vmalle1 instead, or should
the synchronization be upgraded to dsb(ish)?

> +	} else {
> +		switch (flags & (TLBF_NOWALKCACHE | TLBF_NOBROADCAST)) {
> +		case TLBF_NONE:
[ ... ]
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 9142820160698..beb658a007d59 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -585,6 +585,12 @@ static void user_cache_maint_handler(unsigned long esr, struct pt_regs *regs)
>  		__user_cache_maint("dc civac", address, ret);
>  		break;
>  	case ESR_ELx_SYS64_ISS_CRM_IC_IVAU:	/* IC IVAU */
> +		if (cpus_have_final_cap(ARM64_WORKAROUND_NXP_ERR050104)) {
> +			/* ERR050104: upgrade IC IVAU to IC IALLUIS */
> +			asm volatile("ic ialluis");
> +			ret = 0;
> +			break;
> +		}

[Severity: High]
Will this silently swallow faults if user space passes an invalid or kernel
pointer?

Normally, __user_cache_maint() checks if the address is >= TASK_SIZE_MAX and
sets -EFAULT, which correctly triggers a SIGSEGV for the offending user-space
application. 

Because this workaround executes the ic ialluis instruction and returns success
(ret = 0) unconditionally, it bypasses the address validation entirely. Can we
preserve the TASK_SIZE_MAX check so that buggy or malicious programs aren't
silently ignored?

>  		__user_cache_maint("ic ivau", address, ret);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1