[PATCH v5 29/39] arm64: Add helpers to probe local CPU for PAC and BTI support

[Ard Biesheuvel]

On Fri, 24 Nov 2023 at 14:48, Marc Zyngier <maz at kernel.org> wrote:
>
> On Fri, 24 Nov 2023 13:08:33 +0000,
> Ard Biesheuvel <ardb at kernel.org> wrote:
> >
> > On Fri, 24 Nov 2023 at 13:37, Marc Zyngier <maz at kernel.org> wrote:
> > >
> > > On Fri, 24 Nov 2023 10:19:09 +0000,
> > > Ard Biesheuvel <ardb at google.com> wrote:
> > > >
> > > > From: Ard Biesheuvel <ardb at kernel.org>
> > > >
> > > > Add some helpers that will be used by the early kernel mapping code to
> > > > check feature support on the local CPU. This permits the early kernel
> > > > mapping to be created with the right attributes, removing the need for
> > > > tearing it down and recreating it.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > > > ---
> > > >  arch/arm64/include/asm/cpufeature.h | 44 ++++++++++++++++++++
> > > >  1 file changed, 44 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > > > index 99c01417e544..301d4d2211d5 100644
> > > > --- a/arch/arm64/include/asm/cpufeature.h
> > > > +++ b/arch/arm64/include/asm/cpufeature.h
> > > > @@ -921,6 +921,50 @@ static inline bool kaslr_disabled_cmdline(void)
> > > >  u32 get_kvm_ipa_limit(void);
> > > >  void dump_cpu_features(void);
> > > >
> > > > +static inline bool cpu_has_bti(void)
> > > > +{
> > > > +     u64 pfr1;
> > > > +
> > > > +     if (!IS_ENABLED(CONFIG_ARM64_BTI))
> > > > +             return false;
> > > > +
> > > > +     pfr1 = read_cpuid(ID_AA64PFR1_EL1);
> > > > +     pfr1 &= ~id_aa64pfr1_override.mask;
> > > > +     pfr1 |= id_aa64pfr1_override.val;
> > >
> > > There is a potential gotcha here. If the override failed because a
> > > filter rejected it, val will be set to full ones for the size of the
> > > failed override field, and the corresponding mask will be set to 0
> > > (see match_options()).
> > >
> > > A safer pattern would be:
> > >
> > >         pfr1 |= id_aa64pfr1_override.val & id_aa64pfr1_override.mask;
> > >
> > > although we currently don't have such a filter for BTI nor PAC, so the
> > > code is OK for now.
> > >
> >
> > This confuses me.
> >
> > What is the point of a value/mask pair if the mask is applied to the
> > value, and not to the quantity that is being overridden? Surely, not
> > setting those bits in 'value' to begin with makes the whole mask
> > redundant, no?
> >
> > If the filter is supposed to prevent the override from taking effect,
> > wouldn't it be better to use 0/0 for the mask/value pair?
> >
> > (/me likely misses some context here)
>
> 0/0 would be fine for the purpose of not applying the override.
> However, this hack is used to report the failed override at boot time
> instead of silently ignoring it (see [1]).
>
> Is it crap? Absolutely. But I didn't have a better idea at the time
> (and still don't). Alternatively, we can drop the reporting and be
> done with it.
>

No I think it is fine, actually. A valid mask should cover at least
the 1 bits in value, so signalling an invalid override this way is
perfectly reasonable.

But applying the mask to value still does not make sense imo. Instead,
I should just check that the override is valid before applying it.