arm64: Move storage of idreg overrides into mmuoff section

Commit Message

Oliver Upton Jan. 30, 2025, 8:46 p.m. UTC

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...

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: [e-mail] # v5.15+
Tested-by: Moritz Fischer <[e-mail]>
Tested-by: Pedro Martelletto <[e-mail]>
Reported-by: Jon Masters <[e-mail]>
Signed-off-by: Oliver Upton <[e-mail]>
---
 arch/arm64/kernel/cpufeature.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)


base-commit: 1dd3393696efba1598aa7692939bba99d0cffae3

Comments

Ard Biesheuvel Jan. 30, 2025, 9:48 p.m. UTC | #1

Hi Oliver,

On Thu, 30 Jan 2025 at 21:47, Oliver Upton <[e-mail]> 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...
>

I'd like to understand this part a bit better: we wipe BSS right
before we parse the idreg overrides, all via the ID map with the
caches enabled. The ftr_override globals are cleaned+invalidated to
the PoC after we populate them. At this point, system_cpucaps is still
cleared, and it gets populated only much later.

So how do you reckon this corruption occurs? Is there a memory type
mismatch between the 1:1 mapping and the kernel virtual mapping?


> 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.
>

This is fine in principle, but I suspect that it could be anywhere as
long as it is inside the kernel image, rather than memory like BSS
that is part of the executable image but not covered by the file.

Patch

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;