linux-kernel - Re: [PATCH] arm64: Move storage of idreg overrides into mmuoff section

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z5yseC1kCyTcwlMy@J2N7QTR9R3>
Date: Fri, 31 Jan 2025 10:56:56 +0000
From: Mark Rutland <mark.rutland@....com>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
	Mark Brown <broonie@...nel.org>, Ard Biesheuvel <ardb@...nel.org>,
	Joey Gouly <joey.gouly@....com>, James Morse <james.morse@....com>,
	stable@...r.kernel.org, Moritz Fischer <moritzf@...gle.com>,
	Pedro Martelletto <martelletto@...gle.com>,
	Jon Masters <jonmasters@...gle.com>
Subject: Re: [PATCH] arm64: Move storage of idreg overrides into mmuoff
 section

Hi Oliver,

On Thu, Jan 30, 2025 at 12:46:15PM -0800, Oliver Upton wrote:
> There are a few places where the idreg overrides are read w/ the MMU
> off, for example the VHE and hVHE checks in __finalise_el2. And while
> the infrastructure gets this _mostly_ right (i.e. does the appropriate
> cache maintenance), the placement of the data itself is problematic and
> could share a cache line with something else.
> 
> Depending on how unforgiving an implementation's handling of mismatched
> attributes is, this could lead to data corruption. In one observed case,
> the system_cpucaps shared a line with arm64_sw_feature_override and the
> cpucaps got nuked after entering the hyp stub...

This doesn't sound right. Non-cacheable/Device reads should not lead to
corruption of a cached copy regardless of whether that cached copy is
clean or dirty.

The corruption suggests that either we're performing a *write* with
mismatched attributes (in which case the use of .mmuoff.data.read below
isn't quite right), or we have a plan invalidate somewhere without a
clean (and e.g. something else might need to be moved into
.mmuoff.data.write).

Seconding Ard's point, I think we need to understand this scenario
better.

To be clear, I think moving all the overrides into .mmuoff.data.read
makes sense, but it doesn't explain the problem above, and it seems like
there must be a latent issue (which this might only mask rather than
solve).

Mark.

> Even though only a few overrides are read without the MMU on, just throw
> the whole lot into the mmuoff section and be done with it.
> 
> Cc: stable@...r.kernel.org # v5.15+
> Tested-by: Moritz Fischer <moritzf@...gle.com>
> Tested-by: Pedro Martelletto <martelletto@...gle.com>
> Reported-by: Jon Masters <jonmasters@...gle.com>
> Signed-off-by: Oliver Upton <oliver.upton@...ux.dev>
> ---
>  arch/arm64/kernel/cpufeature.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index d41128e37701..92506d9f90db 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -755,17 +755,20 @@ static const struct arm64_ftr_bits ftr_raz[] = {
>  #define ARM64_FTR_REG(id, table)		\
>  	__ARM64_FTR_REG_OVERRIDE(#id, id, table, &no_override)
>  
> -struct arm64_ftr_override id_aa64mmfr0_override;
> -struct arm64_ftr_override id_aa64mmfr1_override;
> -struct arm64_ftr_override id_aa64mmfr2_override;
> -struct arm64_ftr_override id_aa64pfr0_override;
> -struct arm64_ftr_override id_aa64pfr1_override;
> -struct arm64_ftr_override id_aa64zfr0_override;
> -struct arm64_ftr_override id_aa64smfr0_override;
> -struct arm64_ftr_override id_aa64isar1_override;
> -struct arm64_ftr_override id_aa64isar2_override;
> -
> -struct arm64_ftr_override arm64_sw_feature_override;
> +#define DEFINE_FTR_OVERRIDE(name)					\
> +	struct arm64_ftr_override __section(".mmuoff.data.read") name
> +
> +DEFINE_FTR_OVERRIDE(id_aa64mmfr0_override);
> +DEFINE_FTR_OVERRIDE(id_aa64mmfr1_override);
> +DEFINE_FTR_OVERRIDE(id_aa64mmfr2_override);
> +DEFINE_FTR_OVERRIDE(id_aa64pfr0_override);
> +DEFINE_FTR_OVERRIDE(id_aa64pfr1_override);
> +DEFINE_FTR_OVERRIDE(id_aa64zfr0_override);
> +DEFINE_FTR_OVERRIDE(id_aa64smfr0_override);
> +DEFINE_FTR_OVERRIDE(id_aa64isar1_override);
> +DEFINE_FTR_OVERRIDE(id_aa64isar2_override);
> +
> +DEFINE_FTR_OVERRIDE(arm64_sw_feature_override);
>  
>  static const struct __ftr_reg_entry {
>  	u32			sys_id;
> 
> base-commit: 1dd3393696efba1598aa7692939bba99d0cffae3
> -- 
> 2.39.5
> 
>