diff mbox series

[v3,01/16] arm64: cpufeature: add pointer auth meta-capabilities

Message ID 1576486038-9899-2-git-send-email-amit.kachhap@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: return address signing | expand

Commit Message

Amit Daniel Kachhap Dec. 16, 2019, 8:47 a.m. UTC 
From: Kristina Martsenko <kristina.martsenko@arm.com>

To enable pointer auth for the kernel, we're going to need to check for
the presence of address auth and generic auth using alternative_if. We
currently have two cpucaps for each, but alternative_if needs to check a
single cpucap. So define meta-capabilities that are present when either
of the current two capabilities is present.

Leave the existing four cpucaps in place, as they are still needed to
check for mismatched systems where one CPU has the architected algorithm
but another has the IMP DEF algorithm.

Note, the meta-capabilities were present before but were removed in
commit a56005d32105 ("arm64: cpufeature: Reduce number of pointer auth
CPU caps from 6 to 4") and commit 1e013d06120c ("arm64: cpufeature: Rework
ptr auth hwcaps using multi_entry_cap_matches"), as they were not needed
then. Note, unlike before, the current patch checks the cpucap values
directly, instead of reading the CPU ID register value.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
[Amit: commit message and macro rebase]
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
---
Changes since last version:
* Macro number.

 arch/arm64/include/asm/cpucaps.h    |  4 +++-
 arch/arm64/include/asm/cpufeature.h |  6 ++----
 arch/arm64/kernel/cpufeature.c      | 25 ++++++++++++++++++++++++-
 3 files changed, 29 insertions(+), 6 deletions(-)

Comments

Catalin Marinas Jan. 15, 2020, 12:26 p.m. UTC | #1 
On Mon, Dec 16, 2019 at 02:17:03PM +0530, Amit Daniel Kachhap wrote:
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 04cf64e..cf42c46 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1249,6 +1249,20 @@ static void cpu_enable_address_auth(struct arm64_cpu_capabilities const *cap)
>  sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ENIA | SCTLR_ELx_ENIB |
>         SCTLR_ELx_ENDA | SCTLR_ELx_ENDB);
>  }
> +
> +static bool has_address_auth(const struct arm64_cpu_capabilities *entry,
> +     int __unused)
> +{
> +return cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) ||
> +       cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF);
> +}
> +
> +static bool has_generic_auth(const struct arm64_cpu_capabilities *entry,
> +     int __unused)
> +{
> +return cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_ARCH) ||
> +       cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF);
> +}

Do these rely on the order in which the entries are listed in the
arm64_features[] array? It looks like we do the same for PAN_NOT_UAO but
that's pretty fragile.

I'd prefer if we invoked the
cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH]->matches() directly here, maybe
hidden behind a helper (I couldn't find one at a quick look).

--
Catalin
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Suzuki K Poulose Jan. 15, 2020, 1:52 p.m. UTC | #2 
On 15/01/2020 12:26, Catalin Marinas wrote:
> On Mon, Dec 16, 2019 at 02:17:03PM +0530, Amit Daniel Kachhap wrote:
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 04cf64e..cf42c46 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1249,6 +1249,20 @@ static void cpu_enable_address_auth(struct arm64_cpu_capabilities const *cap)
>>   	sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ENIA | SCTLR_ELx_ENIB |
>>   				       SCTLR_ELx_ENDA | SCTLR_ELx_ENDB);
>>   }
>> +
>> +static bool has_address_auth(const struct arm64_cpu_capabilities *entry,
>> +			     int __unused)
>> +{
>> +	return cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) ||
>> +	       cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF);
>> +}
>> +
>> +static bool has_generic_auth(const struct arm64_cpu_capabilities *entry,
>> +			     int __unused)
>> +{
>> +	return cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_ARCH) ||
>> +	       cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF);
>> +}
> 
> Do these rely on the order in which the entries are listed in the
> arm64_features[] array? It looks like we do the same for PAN_NOT_UAO but
> that's pretty fragile.

Yes, it surely depends on the order in which they are listed.

> 
> I'd prefer if we invoked the
> cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH]->matches() directly here, maybe

Yes, calling the matches(cap, SYSTEM_SCOPE), that should work and is 
much better.

> hidden behind a helper (I couldn't find one at a quick look).
> 

There are no helpers for this operation to  do it on a SYSTEM_SCOPE
and this is only needed for caps dependent on the other caps.

May be we could hide the conversion of the number to "cap" as:

static inline struct arm64_cpu_capabilities *cpu_cap_from_number(int n)
{
	if (n < ARM64_NCAPS)
		return cpu_hwcaps_ptr[n];
	return NULL;
}

And use this for "this_cpu_has_cap()" too.

Suzuki
Catalin Marinas Jan. 15, 2020, 4:01 p.m. UTC | #3 
On Wed, Jan 15, 2020 at 01:52:24PM +0000, Suzuki K Poulose wrote:
> On 15/01/2020 12:26, Catalin Marinas wrote:
> > On Mon, Dec 16, 2019 at 02:17:03PM +0530, Amit Daniel Kachhap wrote:
> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index 04cf64e..cf42c46 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -1249,6 +1249,20 @@ static void cpu_enable_address_auth(struct arm64_cpu_capabilities const *cap)
> > >   	sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ENIA | SCTLR_ELx_ENIB |
> > >   				       SCTLR_ELx_ENDA | SCTLR_ELx_ENDB);
> > >   }
> > > +
> > > +static bool has_address_auth(const struct arm64_cpu_capabilities *entry,
> > > +			     int __unused)
> > > +{
> > > +	return cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) ||
> > > +	       cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF);
> > > +}
> > > +
> > > +static bool has_generic_auth(const struct arm64_cpu_capabilities *entry,
> > > +			     int __unused)
> > > +{
> > > +	return cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_ARCH) ||
> > > +	       cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF);
> > > +}
> > 
> > Do these rely on the order in which the entries are listed in the
> > arm64_features[] array? It looks like we do the same for PAN_NOT_UAO but
> > that's pretty fragile.
> 
> Yes, it surely depends on the order in which they are listed.
> 
> > I'd prefer if we invoked the
> > cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH]->matches() directly here, maybe
> 
> Yes, calling the matches(cap, SYSTEM_SCOPE), that should work and is much
> better.
> 
> > hidden behind a helper (I couldn't find one at a quick look).
> > 
> 
> There are no helpers for this operation to  do it on a SYSTEM_SCOPE
> and this is only needed for caps dependent on the other caps.
> 
> May be we could hide the conversion of the number to "cap" as:
> 
> static inline struct arm64_cpu_capabilities *cpu_cap_from_number(int n)
> {
> 	if (n < ARM64_NCAPS)
> 		return cpu_hwcaps_ptr[n];
> 	return NULL;
> }
> 
> And use this for "this_cpu_has_cap()" too.

I'm not bothered about the cpu_cap_from_number() part. I was actually
thinking of something like the diff below:

-----------8<-------------------------
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2595c2886d3f..2ea4c84fcc8a 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2008,6 +2008,18 @@ bool this_cpu_has_cap(unsigned int n)
 	return false;
 }
 
+static bool system_has_cap(unsigned int n)
+{
+	if (n < ARM64_NCAPS) {
+		const struct arm64_cpu_capabilities *cap = cpu_hwcaps_ptrs[n];
+
+		if (cap)
+			return cap->matches(cap, SCOPE_SYSTEM);
+	}
+
+	return false;
+}
+
 void cpu_set_feature(unsigned int num)
 {
 	WARN_ON(num >= MAX_CPU_FEATURES);
@@ -2081,7 +2093,7 @@ void __init setup_cpu_features(void)
 static bool __maybe_unused
 cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused)
 {
-	return (cpus_have_const_cap(ARM64_HAS_PAN) && !cpus_have_const_cap(ARM64_HAS_UAO));
+	return system_has_cap(ARM64_HAS_PAN) && !system_has_cap(ARM64_HAS_UAO);
 }
 
 static void __maybe_unused cpu_enable_cnp(struct arm64_cpu_capabilities const *cap)
Amit Daniel Kachhap Jan. 16, 2020, 12:35 p.m. UTC | #4 
Hi,

On 1/15/20 9:31 PM, Catalin Marinas wrote:
> On Wed, Jan 15, 2020 at 01:52:24PM +0000, Suzuki K Poulose wrote:
>> On 15/01/2020 12:26, Catalin Marinas wrote:
>>> On Mon, Dec 16, 2019 at 02:17:03PM +0530, Amit Daniel Kachhap wrote:
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index 04cf64e..cf42c46 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -1249,6 +1249,20 @@ static void cpu_enable_address_auth(struct arm64_cpu_capabilities const *cap)
>>>>    	sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ENIA | SCTLR_ELx_ENIB |
>>>>    				       SCTLR_ELx_ENDA | SCTLR_ELx_ENDB);
>>>>    }
>>>> +
>>>> +static bool has_address_auth(const struct arm64_cpu_capabilities *entry,
>>>> +			     int __unused)
>>>> +{
>>>> +	return cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) ||
>>>> +	       cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF);
>>>> +}
>>>> +
>>>> +static bool has_generic_auth(const struct arm64_cpu_capabilities *entry,
>>>> +			     int __unused)
>>>> +{
>>>> +	return cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_ARCH) ||
>>>> +	       cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF);
>>>> +}
>>>
>>> Do these rely on the order in which the entries are listed in the
>>> arm64_features[] array? It looks like we do the same for PAN_NOT_UAO but
>>> that's pretty fragile.
>>
>> Yes, it surely depends on the order in which they are listed.
>>
>>> I'd prefer if we invoked the
>>> cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH]->matches() directly here, maybe
>>
>> Yes, calling the matches(cap, SYSTEM_SCOPE), that should work and is much
>> better.
>>
>>> hidden behind a helper (I couldn't find one at a quick look).
>>>
>>
>> There are no helpers for this operation to  do it on a SYSTEM_SCOPE
>> and this is only needed for caps dependent on the other caps.
>>
>> May be we could hide the conversion of the number to "cap" as:
>>
>> static inline struct arm64_cpu_capabilities *cpu_cap_from_number(int n)
>> {
>> 	if (n < ARM64_NCAPS)
>> 		return cpu_hwcaps_ptr[n];
>> 	return NULL;
>> }
>>
>> And use this for "this_cpu_has_cap()" too.
> 
> I'm not bothered about the cpu_cap_from_number() part. I was actually
> thinking of something like the diff below:
> 
> -----------8<-------------------------
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 2595c2886d3f..2ea4c84fcc8a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2008,6 +2008,18 @@ bool this_cpu_has_cap(unsigned int n)
>   	return false;
>   }
>   
> +static bool system_has_cap(unsigned int n)
> +{
> +	if (n < ARM64_NCAPS) {
> +		const struct arm64_cpu_capabilities *cap = cpu_hwcaps_ptrs[n];
> +
> +		if (cap)
> +			return cap->matches(cap, SCOPE_SYSTEM);
> +	}
> +
> +	return false;
> +}
> +

This patch looks fine. ARM64_HAS_ADDRESS_AUTH_* cpufeature is moved to 
SCOPE_BOOT in the subsequent patches. so instead of system_has_cap, 
existing this_cpu_has_cap can be used. This new function can still be 
used for the other system meta capability cpufeatures.

>   void cpu_set_feature(unsigned int num)
>   {
>   	WARN_ON(num >= MAX_CPU_FEATURES);
> @@ -2081,7 +2093,7 @@ void __init setup_cpu_features(void)
>   static bool __maybe_unused
>   cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused)
>   {
> -	return (cpus_have_const_cap(ARM64_HAS_PAN) && !cpus_have_const_cap(ARM64_HAS_UAO));
> +	return system_has_cap(ARM64_HAS_PAN) && !system_has_cap(ARM64_HAS_UAO);
>   }
>   
>   static void __maybe_unused cpu_enable_cnp(struct arm64_cpu_capabilities const *cap)
>
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index b926838..6674ad1 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -56,7 +56,9 @@ 
 #define ARM64_WORKAROUND_CAVIUM_TX2_219_PRFM	46
 #define ARM64_WORKAROUND_1542419		47
 #define ARM64_WORKAROUND_1319367		48
+#define ARM64_HAS_ADDRESS_AUTH			49
+#define ARM64_HAS_GENERIC_AUTH			50
 
-#define ARM64_NCAPS				49
+#define ARM64_NCAPS				51
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 4261d55..c4e27a1 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -590,15 +590,13 @@  static inline bool system_supports_cnp(void)
 static inline bool system_supports_address_auth(void)
 {
 	return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
-		(cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) ||
-		 cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF));
+		cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH);
 }
 
 static inline bool system_supports_generic_auth(void)
 {
 	return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
-		(cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_ARCH) ||
-		 cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF));
+		cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH);
 }
 
 static inline bool system_uses_irq_prio_masking(void)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 04cf64e..cf42c46 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1249,6 +1249,20 @@  static void cpu_enable_address_auth(struct arm64_cpu_capabilities const *cap)
 	sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ENIA | SCTLR_ELx_ENIB |
 				       SCTLR_ELx_ENDA | SCTLR_ELx_ENDB);
 }
+
+static bool has_address_auth(const struct arm64_cpu_capabilities *entry,
+			     int __unused)
+{
+	return cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) ||
+	       cpus_have_const_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF);
+}
+
+static bool has_generic_auth(const struct arm64_cpu_capabilities *entry,
+			     int __unused)
+{
+	return cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_ARCH) ||
+	       cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF);
+}
 #endif /* CONFIG_ARM64_PTR_AUTH */
 
 #ifdef CONFIG_ARM64_PSEUDO_NMI
@@ -1518,7 +1532,6 @@  static const struct arm64_cpu_capabilities arm64_features[] = {
 		.field_pos = ID_AA64ISAR1_APA_SHIFT,
 		.min_field_value = ID_AA64ISAR1_APA_ARCHITECTED,
 		.matches = has_cpuid_feature,
-		.cpu_enable = cpu_enable_address_auth,
 	},
 	{
 		.desc = "Address authentication (IMP DEF algorithm)",
@@ -1529,6 +1542,11 @@  static const struct arm64_cpu_capabilities arm64_features[] = {
 		.field_pos = ID_AA64ISAR1_API_SHIFT,
 		.min_field_value = ID_AA64ISAR1_API_IMP_DEF,
 		.matches = has_cpuid_feature,
+	},
+	{
+		.capability = ARM64_HAS_ADDRESS_AUTH,
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.matches = has_address_auth,
 		.cpu_enable = cpu_enable_address_auth,
 	},
 	{
@@ -1551,6 +1569,11 @@  static const struct arm64_cpu_capabilities arm64_features[] = {
 		.min_field_value = ID_AA64ISAR1_GPI_IMP_DEF,
 		.matches = has_cpuid_feature,
 	},
+	{
+		.capability = ARM64_HAS_GENERIC_AUTH,
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.matches = has_generic_auth,
+	},
 #endif /* CONFIG_ARM64_PTR_AUTH */
 #ifdef CONFIG_ARM64_PSEUDO_NMI
 	{