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

Fuad Tabba
2026-02-20T15:37:15Z

Hi Marc,

On Fri, 20 Feb 2026 at 14:52, Marc Zyngier <maz@kernel.org> wrote:
>
> 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.

I didn't think that much about it, but I guess features designated as
FTR_HIGHER_SAFE (like SpecSEI) usually represent hardware errata or
critical mitigations. So we should not be hiding them at all.

> >
> > 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);
>

Yes, I think WARN_ON() here is the right call.

That said, I still think you should explicitly short-circuit
update_cpu_ftr_reg() for FTR_ALL_HIDDEN features, in addition to the
WARN_ON(). Relying on arm64_ftr_safe_value() to naturally preserve the
safe_val during secondary CPU boot seems mathematically fragile.

Take MTE_frac as an example. It uses S_ARM64_FTR_BITS and
FTR_LOWER_SAFE with a safe_val of 0. If it were marked FTR_ALL_HIDDEN,
init_cpu_ftr_reg() would prime sys_val with 0. But if a secondary CPU
boots and reports -1 (NI), arm64_ftr_safe_value() will execute min(-1,
0) and return -1. update_cpu_ftr_reg() will then overwrite the primed
safe_val (0) with -1. The "hidden" state established by the boot CPU
is gone, and the feature's hardware state is now exposed globally.

Note that MTE is currently ALL_HIDDEN when configured out, so it's not
totally inconceivable that someone decides to make MTE_frac ALL_HIDDEN
as well. Explicitly short-circuiting for FTR_ALL_HIDDEN features in
update_cpu_ftr_reg() seems to be the safer bet here.

Cheers,
/fuad

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