Re: [PATCH 1/9] arm64: Add logic to fully remove features from sanitised id registers

[Marc Zyngier <maz@kernel.org>]

On Fri, 20 Feb 2026 11:06:03 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> > > > +               switch (ftrp->visibility) {
> > > > +               case FTR_VISIBLE:
> > > > +                       val = arm64_ftr_set_value(ftrp, val, ftr_new);
> > > >                         user_mask |= ftr_mask;
> > > > -               else
> > > > +                       break;
> > > > +               case FTR_ALL_HIDDEN:
> > > > +                       val = arm64_ftr_set_value(ftrp, val, ftrp->safe_val);
> > > > +                       reg->user_val = arm64_ftr_set_value(ftrp,
> > > > +                                                           reg->user_val,
> > > > +                                                           ftrp->safe_val);
> > >
> > > Should we also take the safe value in update_cpu_ftr_reg() for FTR_ALL_HIDDEN?
> >
> > I would expect arm64_ftr_safe_value() to do the right thing at that
> > stage, given that we have primed the boot CPU with the safe value, and
> > that we rely on that bootstrap to make the registers converge towards
> > something safe. This is also what happens for the command-line override.
> >
> > Or have you spotted a case where this go wrong?
> 
> I think so... What if a future FTR_ALL_HIDDEN feature is defined as
> FTR_HIGHER_SAFE? Wouldn't that cause problems on secondary CPUs?
> init_cpu_ftr_reg() primes sys_val with safe_val on the boot CPU,
> update_cpu_ftr_reg() on secondary CPUs compares the hardware value
> (ftr_new) against safe_val (ftr_cur). For FTR_HIGHER_SAFE,
> arm64_ftr_safe_value() returns max(ftr_new, safe_val). Since the
> hardware value is higher, update_cpu_ftr_reg() overwrites sys_val with
> the hardware value, resurrecting the hidden feature globally.

Huh, that's an interesting observation.

SpecSEI is the only case we currently deal with that is
HIGHER_SAFE. But look at what this feature describes: bloody
speculative SErrors! Not taking this into account could be really
deadly, and the kernel really ought to know about it.

>
> The features in this patch are FTR_LOWER_SAFE or FTR_EXACT (which
> happen to sink to safe_val), which is why it's not a problem with
> these current features.

My conclusion is that it is simply not safe to make such feature
conditional in any way. Note that's also the case of for an override:
look at how we will refuse to downgrade a value in init_cpu_ftr_reg():

		if ((ftr_mask & reg->override->mask) == ftr_mask) {
			s64 tmp = arm64_ftr_safe_value(ftrp, ftr_ovr, ftr_new);
			char *str = NULL;

			if (ftr_ovr != tmp) {
				/* Unsafe, remove the override */
				reg->override->mask &= ~ftr_mask;
				reg->override->val &= ~ftr_mask;
				tmp = ftr_ovr;
				str = "ignoring override";
		[...]

I think we must prevent this downgrade the same way, meaning that
ALL_HIDDEN and FTR_HIGHER are mutually exclusive.

How about that:

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d58931e63a0b6..2cae00b4b0c5f 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1067,7 +1067,14 @@ static void init_cpu_ftr_reg(u32 sys_reg, u64 new)
 			user_mask |= ftr_mask;
 			break;
 		case FTR_ALL_HIDDEN:
-			val = arm64_ftr_set_value(ftrp, val, ftrp->safe_val);
+			/*
+			 * ALL_HIDDEN and HIGHER_SAFE are incompatible.
+			 * Only hide from userspace, and log the oddity.
+			 */
+			if (WARN_ON(ftrp->type == FTR_HIGHER_SAFE))
+				val = arm64_ftr_set_value(ftrp, val, ftr_new);
+			else
+				val = arm64_ftr_set_value(ftrp, val, ftrp->safe_val);
 			reg->user_val = arm64_ftr_set_value(ftrp,
 							    reg->user_val,
 							    ftrp->safe_val);

Thanks,

	M.

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