| Message ID | 20250130204614.64621-1-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Move storage of idreg overrides into mmuoff section | expand |
Hi Oliver, On Thu, 30 Jan 2025 at 21:47, Oliver Upton <oliver.upton@linux.dev> 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.
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@vger.kernel.org # v5.15+ > Tested-by: Moritz Fischer <moritzf@google.com> > Tested-by: Pedro Martelletto <martelletto@google.com> > Reported-by: Jon Masters <jonmasters@google.com> > Signed-off-by: Oliver Upton <oliver.upton@linux.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 > >
On Fri, Jan 31, 2025 at 10:56:56AM +0000, Mark Rutland wrote: > 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. Of course. So the write to the idreg override is fine and gets written back after cache maintenance. What's happening afterwards is CPU0 pulls in the line to write to system_cpucaps which also happens to contain arm64_sw_feature_override. That line is in UD state when CPU0 calls HVC_FINALISE_EL2 and goes to EL2 with the MMU off. __finalise_el2() does a load on arm64_sw_feature_override which goes out as a ReadNoSnp. I is the only cache state for this request (IHI 0050G B4.2.1.2) and the SF stops tracking the line, so writeback never actually goes anywhere. This patch might have been a touch premature, let me double check a few things on our side.
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;