linux-kernel - Re: [PATCH] arm64/sme: Move storage of reg_smidr to __cpuinfo_store

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <865xnjsnqo.wl-maz@kernel.org>
Date: Mon, 16 Dec 2024 14:31:43 +0000
From: Marc Zyngier <maz@...nel.org>
To: Mark Rutland <mark.rutland@....com>
Cc: Mark Brown <broonie@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>, Peter Collingbourne <pcc@...gle.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH] arm64/sme: Move storage of reg_smidr to __cpuinfo_store_cpu()

On Mon, 16 Dec 2024 12:38:17 +0000,
Mark Rutland <mark.rutland@....com> wrote:
> 
> On Sat, Dec 14, 2024 at 10:56:13AM +0000, Marc Zyngier wrote:
>
> > Why isn't the following a good enough fix? It makes it plain that
> > boot_cpu_data is only a copy of CPU0's initial boot state.
> > 
> > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> > index d79e88fccdfce..0cbb42fd48850 100644
> > --- a/arch/arm64/kernel/cpuinfo.c
> > +++ b/arch/arm64/kernel/cpuinfo.c
> > @@ -497,6 +497,6 @@ void __init cpuinfo_store_boot_cpu(void)
> >  	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0);
> >  	__cpuinfo_store_cpu(info);
> >  
> > +	init_cpu_features(info);
> >  	boot_cpu_data = *info;
> > -	init_cpu_features(&boot_cpu_data);
> >  }
> 
> I think that change in isolation is fine, but I don't think that's the
> right fix.
> 
> I think that what we did in commit:
> 
>    892f7237b3ff ("arm64: Delay initialisation of cpuinfo_arm64::reg_{zcr,smcr}")
> 
> ... introduces an anti-pattern that'd be nice to avoid. That broke the
> existing split of __cpuinfo_store_cpu() and init_cpu_features(), where
> the former read the ID regs, and the latter set up the features
> *without* altering the copy of the ID regs that was read. i.e.
> init_cpu_features() shouldn't write to its info argument at all.
> 
> I understand that we have to do something as a bodge for broken FW which
> traps SME, but I'd much rather we did that within __cpuinfo_store_cpu().

Honestly, I'd rather revert that patch, together with b3000e2133d8
("arm64: Add the arm64.nosme command line option"). I'm getting tired
of the FW nonsense, and we are only allowing vendors to ship untested
crap.

Furthermore, given the state of SME in the kernel, I don't think this
is makes any difference. So maybe this is the right time to reset
everything to a sane state.

> Can we add something to check whether SME was disabled on the command
> line, and use that in __cpuinfo_store_cpu(), effectively reverting
> 892f7237b3ff?

Maybe, but that'd be before any sanitisation of the overrides, so it
would have to severely limit its scope. Something like this, which I
haven't tested:

diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index d79e88fccdfce..9e9295e045009 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -492,10 +492,22 @@ void cpuinfo_store_cpu(void)
 	update_cpu_features(smp_processor_id(), info, &boot_cpu_data);
 }
 
+static void cpuinfo_apply_overrides(struct cpuinfo_arm64 *info)
+{
+	if (FIELD_GET(ID_AA64PFR0_EL1_SVE, id_aa64pfr0_override.mask) &&
+	    !FIELD_GET(ID_AA64PFR0_EL1_SVE, id_aa64pfr0_override.val))
+		info->reg_id_aa64pfr0 &= ~ID_AA64PFR0_EL1_SVE;
+
+	if (FIELD_GET(ID_AA64PFR1_EL1_SME, id_aa64pfr1_override.mask) &&
+	    !FIELD_GET(ID_AA64PFR1_EL1_SME, id_aa64pfr1_override.val))
+		info->reg_id_aa64pfr1 &= ~ID_AA64PFR1_EL1_SME;
+}
+
 void __init cpuinfo_store_boot_cpu(void)
 {
 	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0);
 	__cpuinfo_store_cpu(info);
+	cpuinfo_apply_overrides(info);
 
 	boot_cpu_data = *info;
 	init_cpu_features(&boot_cpu_data);

But this will have ripple effects on the rest of the override code
(the kernel messages are likely to be wrong).

	M.

-- 
Without deviation from the norm, progress is not possible.