arm64: Fully disable configured-out features

We currently make support for some features such as Pointer Auth, SVE or S1POE a compile time decision.

However, while we hide that feature from userspace when such support is disabled, we still leave the value provided by the HW visible to the rest of the kernel, including KVM.

This has the potential to result in ugly state leakage, as half of the kernel knows about the feature, and the other doesn't.

Short of completely banning such compilation options and restore universal knowledge, introduce the possibility to fully remove such knowledge from the sanitised id registers.

This has more or less the same effect as the idreg override that a user can pass on the command-line, only defined at build-time.

For that purpose, we provide a new macro (FTR_CONFIG()) that defines the behaviour of a feature, both when enabled and disabled.

At this stage, nothing is making use of this anti-feature.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h | 15 ++++++++++-----
 arch/arm64/kernel/cpufeature.c      | 21 ++++++++++++++++-----
 2 files changed, 26 insertions(+), 10 deletions(-)

arch/arm64/include/asm/cpufeature.h +8 -3

index 4de51f8d92cba..2731ea13c2c86 100644
@@ -53,15 +53,20 @@ enum ftr_type {
 #define FTR_SIGNED     true    /* Value should be treated as signed */
 #define FTR_UNSIGNED   false   /* Value should be treated as unsigned */
 
-#define FTR_VISIBLE    true    /* Feature visible to the user space */
-#define FTR_HIDDEN     false   /* Feature is hidden from the user */
+enum ftr_visibility {
+       FTR_HIDDEN,             /* Feature hidden from the user */
+       FTR_ALL_HIDDEN,         /* Feature hidden from kernel, user and KVM */
+       FTR_VISIBLE,            /* Feature visible to all observers */
+};
+
+#define FTR_CONFIG(c, e, d)                            \
+       (IS_ENABLED(c) ? FTR_ ## e : FTR_ ## d)
 
-#define FTR_VISIBLE_IF_IS_ENABLED(config)              \

arch/arm64/kernel/cpufeature.c +1 -1

index c840a93b9ef95..b34a39967d111 100644
@@ -192,7 +192,7 @@ void dump_cpu_features(void)
 #define __ARM64_FTR_BITS(SIGNED, VISIBLE, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
        {                                               \
                .sign = SIGNED,                         \
-               .visible = VISIBLE,                     \
+               .visibility = VISIBLE,                  \
                .strict = STRICT,                       \
                .type = TYPE,                           \
                .shift = SHIFT,                         \

arch/arm64/kernel/cpufeature.c +2 -1

                        strict_mask &= ~ftr_mask;
-               if (ftrp->visible)
+
+               switch (ftrp->visibility) {

arch/arm64/kernel/cpufeature.c +5 -1

+                       break;
+               case FTR_HIDDEN:
+                       val = arm64_ftr_set_value(ftrp, val, ftr_new);
                        reg->user_val = arm64_ftr_set_value(ftrp,
                                                            reg->user_val,
                                                            ftrp->safe_val);
+                       break;
+               }
        }
 
        val &= valid_mask;
--

2.47.3

Hi Marc,

Quoted 52 lines · Marc Zyngier

We currently make support for some features such as Pointer Auth, SVE or S1POE a compile time decision.

However, while we hide that feature from userspace when such support is disabled, we still leave the value provided by the HW visible to the rest of the kernel, including KVM.

This has the potential to result in ugly state leakage, as half of the kernel knows about the feature, and the other doesn't.

Short of completely banning such compilation options and restore universal knowledge, introduce the possibility to fully remove such knowledge from the sanitised id registers.

This has more or less the same effect as the idreg override that a user can pass on the command-line, only defined at build-time.

For that purpose, we provide a new macro (FTR_CONFIG()) that defines the behaviour of a feature, both when enabled and disabled.

At this stage, nothing is making use of this anti-feature.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h | 15 ++++++++++-----
 arch/arm64/kernel/cpufeature.c      | 21 ++++++++++++++++-----
 2 files changed, 26 insertions(+), 10 deletions(-)

arch/arm64/include/asm/cpufeature.h +9 -4

index 4de51f8d92cba..2731ea13c2c86 100644
@@ -53,15 +53,20 @@ enum ftr_type {
 #define FTR_SIGNED     true    /* Value should be treated as signed */
 #define FTR_UNSIGNED   false   /* Value should be treated as unsigned */
 
-#define FTR_VISIBLE    true    /* Feature visible to the user space */
-#define FTR_HIDDEN     false   /* Feature is hidden from the user */
+enum ftr_visibility {
+       FTR_HIDDEN,             /* Feature hidden from the user */
+       FTR_ALL_HIDDEN,         /* Feature hidden from kernel, user and KVM */
+       FTR_VISIBLE,            /* Feature visible to all observers */
+};
+
+#define FTR_CONFIG(c, e, d)                            \
+       (IS_ENABLED(c) ? FTR_ ## e : FTR_ ## d)
 
-#define FTR_VISIBLE_IF_IS_ENABLED(config)              \
-       (IS_ENABLED(config) ? FTR_VISIBLE : FTR_HIDDEN)
+#define FTR_VISIBLE_IF_IS_ENABLED(c)   FTR_CONFIG(c, VISIBLE, HIDDEN)
 

arch/arm64/include/asm/cpufeature.h enum ftr_type { · line 72

Quoted 7 lines

arch/arm64/include/asm/cpufeature.h +1 -1

 struct arm64_ftr_bits {
        bool            sign;   /* Value is signed ? */
-       bool            visible;
+       enum ftr_visibility visibility;
        bool            strict; /* CPU Sanity check: strict matching required ? */
        enum ftr_type   type;
        u8              shift;

This introduces bloat. Should you group the bools together and the enums together?

Quoted 15 lines

arch/arm64/kernel/cpufeature.c +1 -1

index c840a93b9ef95..b34a39967d111 100644
@@ -192,7 +192,7 @@ void dump_cpu_features(void)
 #define __ARM64_FTR_BITS(SIGNED, VISIBLE, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
        {                                               \
                .sign = SIGNED,                         \
-               .visible = VISIBLE,                     \
+               .visibility = VISIBLE,                  \
                .strict = STRICT,                       \
                .type = TYPE,                           \
                .shift = SHIFT,                         \

arch/arm64/kernel/cpufeature.c static void init_cpu_ftr_reg(u32 sys_reg, u64 new) · line 1061

Quoted 7 lines

arch/arm64/kernel/cpufeature.c +0 -2

@@ -1057,17 +1057,28 @@ static void init_cpu_ftr_reg(u32 sys_reg, u64 new)
                                ftrp->shift);
                }
 
-               val = arm64_ftr_set_value(ftrp, val, ftr_new);
-
                valid_mask |= ftr_mask;
                if (!ftrp->strict)

Should FTR_ALL_HIDDEN also be removed from strict_mask? i.e.

(quoted diff) +1 -1

-                 if (!ftrp->strict)
+                 if (!ftrp->strict || || ftrp->visibility == FTR_ALL_HIDDEN)

(or under the ALL_HIDDEN case below).

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +2 -1

                        strict_mask &= ~ftr_mask;
-               if (ftrp->visible)
+
+               switch (ftrp->visibility) {

arch/arm64/kernel/cpufeature.c static void init_cpu_ftr_reg(u32 sys_reg, u64 new) · line 1073

Quoted 10 lines

arch/arm64/kernel/cpufeature.c +8 -1

+               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?

Hey Fuad,

Thanks for taking a look.

Quoted 56 lines · Fuad Tabba

Hi Marc,

Quoted 52 lines · Marc Zyngier

We currently make support for some features such as Pointer Auth, SVE or S1POE a compile time decision.

However, while we hide that feature from userspace when such support is disabled, we still leave the value provided by the HW visible to the rest of the kernel, including KVM.

This has the potential to result in ugly state leakage, as half of the kernel knows about the feature, and the other doesn't.

Short of completely banning such compilation options and restore universal knowledge, introduce the possibility to fully remove such knowledge from the sanitised id registers.

This has more or less the same effect as the idreg override that a user can pass on the command-line, only defined at build-time.

For that purpose, we provide a new macro (FTR_CONFIG()) that defines the behaviour of a feature, both when enabled and disabled.

At this stage, nothing is making use of this anti-feature.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h | 15 ++++++++++-----
 arch/arm64/kernel/cpufeature.c      | 21 ++++++++++++++++-----
 2 files changed, 26 insertions(+), 10 deletions(-)

arch/arm64/include/asm/cpufeature.h +9 -4

index 4de51f8d92cba..2731ea13c2c86 100644
@@ -53,15 +53,20 @@ enum ftr_type {
 #define FTR_SIGNED     true    /* Value should be treated as signed */
 #define FTR_UNSIGNED   false   /* Value should be treated as unsigned */
 
-#define FTR_VISIBLE    true    /* Feature visible to the user space */
-#define FTR_HIDDEN     false   /* Feature is hidden from the user */
+enum ftr_visibility {
+       FTR_HIDDEN,             /* Feature hidden from the user */
+       FTR_ALL_HIDDEN,         /* Feature hidden from kernel, user and KVM */
+       FTR_VISIBLE,            /* Feature visible to all observers */
+};
+
+#define FTR_CONFIG(c, e, d)                            \
+       (IS_ENABLED(c) ? FTR_ ## e : FTR_ ## d)
 
-#define FTR_VISIBLE_IF_IS_ENABLED(config)              \
-       (IS_ENABLED(config) ? FTR_VISIBLE : FTR_HIDDEN)
+#define FTR_VISIBLE_IF_IS_ENABLED(c)   FTR_CONFIG(c, VISIBLE, HIDDEN)
 

Quoted 10 lines

arch/arm64/include/asm/cpufeature.h enum ftr_visibility { · line 70

Quoted 7 lines

arch/arm64/include/asm/cpufeature.h +1 -1

 struct arm64_ftr_bits {
        bool            sign;   /* Value is signed ? */
-       bool            visible;
+       enum ftr_visibility visibility;
        bool            strict; /* CPU Sanity check: strict matching required ? */
        enum ftr_type   type;
        u8              shift;

This introduces bloat. Should you group the bools together and the enums together?

That should be possible as long as everybody ends up using the __ARM64_FTR_BITS macro. On the other hand, I could reduce the width of the enum to something more appropriate and keep the current layout.

With the current changes, this looks like this:

struct arm64_ftr_bits {
bool sign; /* 0 1 */

/* XXX 3 bytes hole, try to pack */

enum ftr_visibility visibility; /* 4 4 */
bool strict; /* 8 1 */

/* XXX 3 bytes hole, try to pack */

enum ftr_type type; /* 12 4 */
u8 shift; /* 16 1 */
u8 width; /* 17 1 */

/* XXX 6 bytes hole, try to pack */

s64 safe_val; /* 24 8 */

/* size: 32, cachelines: 1, members: 7 */ /* sum members: 20, holes: 3, sum holes: 12 */ /* last cacheline: 32 bytes */
};

which is 8 bytes larger than the upstream version. But if I do this:

arch/arm64/include/asm/cpufeature.h +2 -2

index adaae3060851c..d8accc9c94fab 100644
@@ -64,9 +64,9 @@ enum ftr_visibility {
 
 struct arm64_ftr_bits {
        bool            sign;   /* Value is signed ? */
-       enum ftr_visibility visibility;
+       enum ftr_visibility visibility:8;
        bool            strict; /* CPU Sanity check: strict matching required ? */
-       enum ftr_type   type;
+       enum ftr_type   type:8;
        u8              shift;
        u8              width;
        s64             safe_val; /* safe value for FTR_EXACT features */

I end up with the following layout:

struct arm64_ftr_bits {
        bool                       sign;                 /*     0     1 */

        /* Bitfield combined with previous fields */

        enum ftr_visibility        visibility:8;         /*     0: 8  4 */

        /* Bitfield combined with next fields */

        bool                       strict;               /*     2     1 */

        /* Bitfield combined with previous fields */

        enum ftr_type              type:8;               /*     0:24  4 */
        u8                         shift;                /*     4     1 */
        u8                         width;                /*     5     1 */

        /* XXX 2 bytes hole, try to pack */

        s64                        safe_val;             /*     8     8 */

        /* size: 16, cachelines: 1, members: 7 */
        /* sum members: 12, holes: 1, sum holes: 2 */
        /* sum bitfield members: 16 bits (2 bytes) */
        /* last cacheline: 16 bytes */
};

which is 8 bytes *smaller* than the upstream version, without reordering. WDYT?

Quoted 29 lines

Quoted 16 lines

arch/arm64/kernel/cpufeature.c +1 -1

index c840a93b9ef95..b34a39967d111 100644
@@ -192,7 +192,7 @@ void dump_cpu_features(void)
 #define __ARM64_FTR_BITS(SIGNED, VISIBLE, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
        {                                               \
                .sign = SIGNED,                         \
-               .visible = VISIBLE,                     \
+               .visibility = VISIBLE,                  \
                .strict = STRICT,                       \
                .type = TYPE,                           \
                .shift = SHIFT,                         \

arch/arm64/kernel/cpufeature.c static void init_cpu_ftr_reg(u32 sys_reg, u64 new) · line 1061

Quoted 7 lines

arch/arm64/kernel/cpufeature.c +0 -2

@@ -1057,17 +1057,28 @@ static void init_cpu_ftr_reg(u32 sys_reg, u64 new)
                                ftrp->shift);
                }
 
-               val = arm64_ftr_set_value(ftrp, val, ftr_new);
-
                valid_mask |= ftr_mask;
                if (!ftrp->strict)

Should FTR_ALL_HIDDEN also be removed from strict_mask? i.e.

(quoted diff) +1 -1

-                 if (!ftrp->strict)
+                 if (!ftrp->strict || || ftrp->visibility == FTR_ALL_HIDDEN)

Quoted 1 line

(or under the ALL_HIDDEN case below).

If we run on a system that has diverging features, we probably want to know irrespective of the feature being enabled. After all, the integration is out of spec, and conveying that information is important, just in case the diverging feature affects behaviour in funny ways...

Quoted 17 lines

Quoted 16 lines

arch/arm64/kernel/cpufeature.c +10 -2

                        strict_mask &= ~ftr_mask;
-               if (ftrp->visible)
+
+               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);

Quoted 1 line

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?

Thanks,

        M.

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

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

(quoted diff) +9 -1

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

Quoted 8 lines

Quoted 1 line

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.

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.

Quoted 22 lines · Fuad Tabba

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

(quoted diff) +9 -1

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

Quoted 8 lines

Quoted 1 line

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?

Quoted 8 lines

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.

Quoted 4 lines

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:

arch/arm64/kernel/cpufeature.c +1 -1

index d58931e63a0b6..2cae00b4b0c5f 100644
@@ -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);
+                       /*

        M.

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

Quoted 17 lines

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

How about that:

arch/arm64/kernel/cpufeature.c +1 -1

index d58931e63a0b6..2cae00b4b0c5f 100644
@@ -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);
+                       /*

arch/arm64/kernel/cpufeature.c static void init_cpu_ftr_reg(u32 sys_reg, u64 new) · line 1080

Quoted 10 lines

arch/arm64/kernel/cpufeature.c +7 -0

+                        * 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.

Hi Fuad,

Quoted 43 lines · Fuad Tabba

Quoted 17 lines

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

How about that:

arch/arm64/kernel/cpufeature.c +1 -1

index d58931e63a0b6..2cae00b4b0c5f 100644
@@ -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);
+                       /*

arch/arm64/kernel/cpufeature.c static void init_cpu_ftr_reg(u32 sys_reg, u64 new) · line 1080

Quoted 10 lines

arch/arm64/kernel/cpufeature.c +7 -0

+                        * 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.

Quoted 4 lines

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.

Right, the signed feature is a pretty compelling argument. And we should do the same thing for overrides, probably as a preliminary patch.

	M.