Re: [PATCH v2 3/4] arm64: cpufeature: Modify address authentication cpufeature to exact

[Amit Kachhap <amit.kachhap@arm.com>]

On 5/12/20 11:03 PM, Catalin Marinas wrote:
> On Fri, May 08, 2020 at 09:51:53PM +0530, Amit Kachhap wrote:
>> On 5/6/20 10:43 PM, Catalin Marinas wrote:
>>> On Tue, Apr 14, 2020 at 11:01:53AM +0530, Amit Daniel Kachhap wrote:
>>>> This patch modifies the address authentication cpufeature type to EXACT
>>>> from earlier LOWER_SAFE as the different configurations added for Armv8.6
>>>> enhanced PAC have different behaviour and there is no tunable to enable the
>>>> lower safe versions. The safe value is set as 0.
>>>>
>>>> After this change, if there is any variation in configurations in secondary
>>>> cpus from boot cpu then those cpus are marked tainted. The KVM guests may
>>>> completely disable address authentication if there is any such variations
>>>> detected.
>>>>
>>>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>>>> ---
>>>>    arch/arm64/kernel/cpufeature.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index 08795025409c..599b03df2f93 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -154,9 +154,9 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>>>>    	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_FCMA_SHIFT, 4, 0),
>>>>    	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_JSCVT_SHIFT, 4, 0),
>>>>    	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>>>> -		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_API_SHIFT, 4, 0),
>>>> +		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR1_API_SHIFT, 4, 0),
>>>>    	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>>>> -		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_APA_SHIFT, 4, 0),
>>>> +		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR1_APA_SHIFT, 4, 0),
>>>>    	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_DPB_SHIFT, 4, 0),
>>>>    	ARM64_FTR_END,
>>>
>>> Is this sufficient? If we have the boot CPU already enabling the ptrauth
>>> and we get a secondary CPU with a different ISAR1 field that matches the
>>> address auth in cpufeature.c, we still allow it to boot. We no longer
>>> report the feature to the user system_supports_address_auth() is true
>>> while system_supports_generic_auth() would be false as it checks the
>>> sanitised feature registers.
>>
>> Yes agreed. Generic authentication also needs EXACT cpufeature type.
> 
> I'm still not sure that's sufficient. If we boot the primary CPU with
> ptrauth as detected in proc.S, we consider this a boot feature so all
> secondary CPUs must have it. Subsequent CPUs are currently checked via
> the arm64_features[] definitions and we allow them to boot if the ID is
> at least that of the boot CPU. How does this interact with the above
> FTR_EXACT changes?

Unfortunately FTR_EXACT does not effect the bootflow directly but marks
the cpu TAINTED and goes ahead.

> 
> My concern is that we boot with PAC enabled on all CPUs but because of
> the FTR_EXACT, the sanitised ID registers no longer report the feature.
> 

You are right that PAC is enabled in hardware but un-reported to user in 
this case.

The issue here is in feature_matches() which only validates with the
entry->min_field_value. If we can modify this value to boot cpu value
for FTR_EXACT type then this cpu will fail to online.
May be we can introduce a new structure or make arm64_feature[] writable 
for this.

Something like below code.
-------------------------------------------------------------------------
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 3ae35aadbc20..967928568edf 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -103,6 +103,8 @@ DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
  bool arm64_use_ng_mappings = false;
  EXPORT_SYMBOL(arm64_use_ng_mappings);

+u8 min_cap_value[ARM64_NCAPS];
+
  /*
   * Flag to indicate if we have computed the system wide
   * capabilities based on the boot time active CPUs. This
@@ -616,6 +618,17 @@ static void __init sort_ftr_regs(void)
                 BUG_ON(arm64_ftr_regs[i].sys_id < arm64_ftr_regs[i - 
1].sys_id);
  }

+static void __init update_cpu_capability_min(u32 sys_reg, s64 new)
+{
+       const struct arm64_cpu_capabilities *caps = arm64_features;
+       for (; caps->matches; caps++) {
+               if (caps->sys_reg == sys_reg) {
+                       if (caps->min_field_value)
+                               min_cap_value[caps->capability] = new;
+               }
+       }
+}
+
  /*
   * Initialise the CPU feature register from Boot CPU values.
   * Also initiliases the strict_mask for the register.
@@ -649,6 +662,8 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 
new)
                         reg->user_val = arm64_ftr_set_value(ftrp,
                                                             reg->user_val,
 
ftrp->safe_val);
+               if (ftrp->type == FTR_EXACT)
+                       update_cpu_capability_min(sys_reg, ftr_new);
         }

         val &= valid_mask;
@@ -1021,8 +1036,10 @@ static bool
  feature_matches(u64 reg, const struct arm64_cpu_capabilities *entry)
  {
         int val = cpuid_feature_extract_field(reg, entry->field_pos, 
entry->sign);
-
-       return val >= entry->min_field_value;
+       if (min_cap_value[entry->capability])
+               return val >= min_cap_value[entry->capability];
+       else
+               return val >= entry->min_field_value;
  }

  static bool

-------------------------------------------------------------------------

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel