Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-02-08 Thread Dave Martin 
On Wed, Feb 07, 2018 at 06:15:58PM +, Suzuki K Poulose wrote:
> On 07/02/18 10:38, Dave Martin wrote:
> >On Wed, Jan 31, 2018 at 06:27:57PM +, Suzuki K Poulose wrote:
> >>KPTI is treated as a system wide feature, where we enable the feature
> >>when all the CPUs on the system suffers from the security vulnerability,
> >
> >Should that be "when any CPU"?
> >
> 
> Without this patch, we need all the CPUs to mandate the defense (as this
> is a system feature). This patch changes it. I will change it to :
> 
> "KPTI is treated as a system wide feature and is only "detected" if all
> the CPUs on the system needs the defense. This is not sufficient, as the
> KPTI is turned off on a system with a mix of CPUs, where some CPUs can
> defend and others can't,
> 
> >>unless it is forced via kernel command line. Also, if a late CPU needs
> >>KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
> >>to boot, which is a potential security vulnerability.  This patch ensures
> 
> " This patch ensures that KPTI is turned on if at least one CPU requires the
> defense and any late CPUs are rejected..."
> .

Yes, that makes sense.

[...]

> >>diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>index ecc87aa74c64..4a55492784b7 100644
> >>--- a/arch/arm64/kernel/cpufeature.c
> >>+++ b/arch/arm64/kernel/cpufeature.c

[...]

> >>@@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
> >>arm64_features[] = {
> >>{
> >>.desc = "Kernel page table isolation (KPTI)",
> >>.capability = ARM64_UNMAP_KERNEL_AT_EL0,
> >>-   .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> >>+   .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
> >>+   .sys_reg = SYS_ID_AA64PFR0_EL1,
> >>+   .field_pos = ID_AA64PFR0_CSV3_SHIFT,
> >>+   .min_field_value = 1,
> >>.matches = unmap_kernel_at_el0,
> >
> >Minor nit, but:
> >
> >Can we have a comment here to explain that .min_field_value is the
> >minimum value that indicates that KPTI is _not_ required by this cpu?
> >This is the opposite of the usual semantics for this field.
> 
> Sure, will add it.
> 
> >
> >Otherwise, this inversion of meaning is not obvious without digging into
> >unmap_kernel_at_el0() and spotting the ! in !has_cpuid_feature().
> >
> >With that, or if this usage of !has_cpuid_feature() is already well-
> >established so that a comment is deemed unnecessary:
> 
> This is the first time we have used it.

Thought so, but I wasn't ruling out the possibility that I had missed
it!

Cheers
---Dave

Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-02-08 Thread Dave Martin 
On Wed, Feb 07, 2018 at 06:15:58PM +, Suzuki K Poulose wrote:
> On 07/02/18 10:38, Dave Martin wrote:
> >On Wed, Jan 31, 2018 at 06:27:57PM +, Suzuki K Poulose wrote:
> >>KPTI is treated as a system wide feature, where we enable the feature
> >>when all the CPUs on the system suffers from the security vulnerability,
> >
> >Should that be "when any CPU"?
> >
> 
> Without this patch, we need all the CPUs to mandate the defense (as this
> is a system feature). This patch changes it. I will change it to :
> 
> "KPTI is treated as a system wide feature and is only "detected" if all
> the CPUs on the system needs the defense. This is not sufficient, as the
> KPTI is turned off on a system with a mix of CPUs, where some CPUs can
> defend and others can't,
> 
> >>unless it is forced via kernel command line. Also, if a late CPU needs
> >>KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
> >>to boot, which is a potential security vulnerability.  This patch ensures
> 
> " This patch ensures that KPTI is turned on if at least one CPU requires the
> defense and any late CPUs are rejected..."
> .

Yes, that makes sense.

[...]

> >>diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>index ecc87aa74c64..4a55492784b7 100644
> >>--- a/arch/arm64/kernel/cpufeature.c
> >>+++ b/arch/arm64/kernel/cpufeature.c

[...]

> >>@@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
> >>arm64_features[] = {
> >>{
> >>.desc = "Kernel page table isolation (KPTI)",
> >>.capability = ARM64_UNMAP_KERNEL_AT_EL0,
> >>-   .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> >>+   .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
> >>+   .sys_reg = SYS_ID_AA64PFR0_EL1,
> >>+   .field_pos = ID_AA64PFR0_CSV3_SHIFT,
> >>+   .min_field_value = 1,
> >>.matches = unmap_kernel_at_el0,
> >
> >Minor nit, but:
> >
> >Can we have a comment here to explain that .min_field_value is the
> >minimum value that indicates that KPTI is _not_ required by this cpu?
> >This is the opposite of the usual semantics for this field.
> 
> Sure, will add it.
> 
> >
> >Otherwise, this inversion of meaning is not obvious without digging into
> >unmap_kernel_at_el0() and spotting the ! in !has_cpuid_feature().
> >
> >With that, or if this usage of !has_cpuid_feature() is already well-
> >established so that a comment is deemed unnecessary:
> 
> This is the first time we have used it.

Thought so, but I wasn't ruling out the possibility that I had missed
it!

Cheers
---Dave

Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-02-07 Thread Suzuki K Poulose 

On 07/02/18 10:38, Dave Martin wrote:

On Wed, Jan 31, 2018 at 06:27:57PM +, Suzuki K Poulose wrote:

KPTI is treated as a system wide feature, where we enable the feature
when all the CPUs on the system suffers from the security vulnerability,


Should that be "when any CPU"?



Without this patch, we need all the CPUs to mandate the defense (as this
is a system feature). This patch changes it. I will change it to :

"KPTI is treated as a system wide feature and is only "detected" if all
the CPUs on the system needs the defense. This is not sufficient, as the
KPTI is turned off on a system with a mix of CPUs, where some CPUs can
defend and others can't,


unless it is forced via kernel command line. Also, if a late CPU needs
KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
to boot, which is a potential security vulnerability.  This patch ensures


" This patch ensures that KPTI is turned on if at least one CPU requires the
defense and any late CPUs are rejected..."
.

that late CPUs are rejected as appropriate if they need KPTI but it wasn't
enabled.

Cc: Will Deacon 
Cc: Dave Martin 
Signed-off-by: Suzuki K Poulose 
---
  arch/arm64/include/asm/cpufeature.h |  9 +
  arch/arm64/kernel/cpufeature.c  | 11 ++-
  2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 7bb3fdec827e..71993dd4afae 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -223,6 +223,15 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
 ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU |   \
 ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU)
  
+/*

+ * CPU feature detected at boot time, on one or more CPUs. A late CPU
+ * is not allowed to have the capability when the system doesn't have it.
+ * It is Ok for a late CPU to miss the feature.
+ */
+#define ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE \
+   (ARM64_CPUCAP_SCOPE_LOCAL_CPU   |   \
+ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU)
+
  struct arm64_cpu_capabilities {
const char *desc;
u16 capability;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index ecc87aa74c64..4a55492784b7 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -862,9 +862,8 @@ static bool has_no_fpsimd(const struct 
arm64_cpu_capabilities *entry, int __unus
  static int __kpti_forced; /* 0: not forced, >0: forced on, <0: forced off */
  
  static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,

-   int __unused)
+   int scope)
  {
-   u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
  
  	/* Forced on command line? */

if (__kpti_forced) {
@@ -885,8 +884,7 @@ static bool unmap_kernel_at_el0(const struct 
arm64_cpu_capabilities *entry,
}
  
  	/* Defer to CPU feature registers */

-   return !cpuid_feature_extract_unsigned_field(pfr0,
-ID_AA64PFR0_CSV3_SHIFT);
+   return !has_cpuid_feature(entry, scope);
  }
  
  static int __init parse_kpti(char *str)

@@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
arm64_features[] = {
{
.desc = "Kernel page table isolation (KPTI)",
.capability = ARM64_UNMAP_KERNEL_AT_EL0,
-   .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+   .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
+   .sys_reg = SYS_ID_AA64PFR0_EL1,
+   .field_pos = ID_AA64PFR0_CSV3_SHIFT,
+   .min_field_value = 1,
.matches = unmap_kernel_at_el0,


Minor nit, but:

Can we have a comment here to explain that .min_field_value is the
minimum value that indicates that KPTI is _not_ required by this cpu?
This is the opposite of the usual semantics for this field.


Sure, will add it.



Otherwise, this inversion of meaning is not obvious without digging into
unmap_kernel_at_el0() and spotting the ! in !has_cpuid_feature().

With that, or if this usage of !has_cpuid_feature() is already well-
established so that a comment is deemed unnecessary:


This is the first time we have used it.

Cheers
Suzuki

Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-02-07 Thread Suzuki K Poulose 

On 07/02/18 10:38, Dave Martin wrote:

On Wed, Jan 31, 2018 at 06:27:57PM +, Suzuki K Poulose wrote:

KPTI is treated as a system wide feature, where we enable the feature
when all the CPUs on the system suffers from the security vulnerability,


Should that be "when any CPU"?



Without this patch, we need all the CPUs to mandate the defense (as this
is a system feature). This patch changes it. I will change it to :

"KPTI is treated as a system wide feature and is only "detected" if all
the CPUs on the system needs the defense. This is not sufficient, as the
KPTI is turned off on a system with a mix of CPUs, where some CPUs can
defend and others can't,


unless it is forced via kernel command line. Also, if a late CPU needs
KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
to boot, which is a potential security vulnerability.  This patch ensures


" This patch ensures that KPTI is turned on if at least one CPU requires the
defense and any late CPUs are rejected..."
.

that late CPUs are rejected as appropriate if they need KPTI but it wasn't
enabled.

Cc: Will Deacon 
Cc: Dave Martin 
Signed-off-by: Suzuki K Poulose 
---
  arch/arm64/include/asm/cpufeature.h |  9 +
  arch/arm64/kernel/cpufeature.c  | 11 ++-
  2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 7bb3fdec827e..71993dd4afae 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -223,6 +223,15 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
 ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU |   \
 ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU)
  
+/*

+ * CPU feature detected at boot time, on one or more CPUs. A late CPU
+ * is not allowed to have the capability when the system doesn't have it.
+ * It is Ok for a late CPU to miss the feature.
+ */
+#define ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE \
+   (ARM64_CPUCAP_SCOPE_LOCAL_CPU   |   \
+ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU)
+
  struct arm64_cpu_capabilities {
const char *desc;
u16 capability;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index ecc87aa74c64..4a55492784b7 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -862,9 +862,8 @@ static bool has_no_fpsimd(const struct 
arm64_cpu_capabilities *entry, int __unus
  static int __kpti_forced; /* 0: not forced, >0: forced on, <0: forced off */
  
  static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,

-   int __unused)
+   int scope)
  {
-   u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
  
  	/* Forced on command line? */

if (__kpti_forced) {
@@ -885,8 +884,7 @@ static bool unmap_kernel_at_el0(const struct 
arm64_cpu_capabilities *entry,
}
  
  	/* Defer to CPU feature registers */

-   return !cpuid_feature_extract_unsigned_field(pfr0,
-ID_AA64PFR0_CSV3_SHIFT);
+   return !has_cpuid_feature(entry, scope);
  }
  
  static int __init parse_kpti(char *str)

@@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
arm64_features[] = {
{
.desc = "Kernel page table isolation (KPTI)",
.capability = ARM64_UNMAP_KERNEL_AT_EL0,
-   .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+   .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
+   .sys_reg = SYS_ID_AA64PFR0_EL1,
+   .field_pos = ID_AA64PFR0_CSV3_SHIFT,
+   .min_field_value = 1,
.matches = unmap_kernel_at_el0,


Minor nit, but:

Can we have a comment here to explain that .min_field_value is the
minimum value that indicates that KPTI is _not_ required by this cpu?
This is the opposite of the usual semantics for this field.


Sure, will add it.



Otherwise, this inversion of meaning is not obvious without digging into
unmap_kernel_at_el0() and spotting the ! in !has_cpuid_feature().

With that, or if this usage of !has_cpuid_feature() is already well-
established so that a comment is deemed unnecessary:


This is the first time we have used it.

Cheers
Suzuki

Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-02-07 Thread Dave Martin 
On Wed, Jan 31, 2018 at 06:27:57PM +, Suzuki K Poulose wrote:
> KPTI is treated as a system wide feature, where we enable the feature
> when all the CPUs on the system suffers from the security vulnerability,

Should that be "when any CPU"?

> unless it is forced via kernel command line. Also, if a late CPU needs
> KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
> to boot, which is a potential security vulnerability.  This patch ensures
> that late CPUs are rejected as appropriate if they need KPTI but it wasn't
> enabled.
> 
> Cc: Will Deacon 
> Cc: Dave Martin 
> Signed-off-by: Suzuki K Poulose 
> ---
>  arch/arm64/include/asm/cpufeature.h |  9 +
>  arch/arm64/kernel/cpufeature.c  | 11 ++-
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h 
> b/arch/arm64/include/asm/cpufeature.h
> index 7bb3fdec827e..71993dd4afae 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -223,6 +223,15 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU |   \
>ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU)
>  
> +/*
> + * CPU feature detected at boot time, on one or more CPUs. A late CPU
> + * is not allowed to have the capability when the system doesn't have it.
> + * It is Ok for a late CPU to miss the feature.
> + */
> +#define ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE   \
> + (ARM64_CPUCAP_SCOPE_LOCAL_CPU   |   \
> +  ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU)
> +
>  struct arm64_cpu_capabilities {
>   const char *desc;
>   u16 capability;
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index ecc87aa74c64..4a55492784b7 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -862,9 +862,8 @@ static bool has_no_fpsimd(const struct 
> arm64_cpu_capabilities *entry, int __unus
>  static int __kpti_forced; /* 0: not forced, >0: forced on, <0: forced off */
>  
>  static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
> - int __unused)
> + int scope)
>  {
> - u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
>  
>   /* Forced on command line? */
>   if (__kpti_forced) {
> @@ -885,8 +884,7 @@ static bool unmap_kernel_at_el0(const struct 
> arm64_cpu_capabilities *entry,
>   }
>  
>   /* Defer to CPU feature registers */
> - return !cpuid_feature_extract_unsigned_field(pfr0,
> -  ID_AA64PFR0_CSV3_SHIFT);
> + return !has_cpuid_feature(entry, scope);
>  }
>  
>  static int __init parse_kpti(char *str)
> @@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
> arm64_features[] = {
>   {
>   .desc = "Kernel page table isolation (KPTI)",
>   .capability = ARM64_UNMAP_KERNEL_AT_EL0,
> - .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> + .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
> + .sys_reg = SYS_ID_AA64PFR0_EL1,
> + .field_pos = ID_AA64PFR0_CSV3_SHIFT,
> + .min_field_value = 1,
>   .matches = unmap_kernel_at_el0,

Minor nit, but:

Can we have a comment here to explain that .min_field_value is the
minimum value that indicates that KPTI is _not_ required by this cpu?
This is the opposite of the usual semantics for this field.

Otherwise, this inversion of meaning is not obvious without digging into
unmap_kernel_at_el0() and spotting the ! in !has_cpuid_feature().

With that, or if this usage of !has_cpuid_feature() is already well-
established so that a comment is deemed unnecessary:

Reviewed-by: Dave Martin 

Cheers
---Dave

Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-02-07 Thread Dave Martin 
On Wed, Jan 31, 2018 at 06:27:57PM +, Suzuki K Poulose wrote:
> KPTI is treated as a system wide feature, where we enable the feature
> when all the CPUs on the system suffers from the security vulnerability,

Should that be "when any CPU"?

> unless it is forced via kernel command line. Also, if a late CPU needs
> KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
> to boot, which is a potential security vulnerability.  This patch ensures
> that late CPUs are rejected as appropriate if they need KPTI but it wasn't
> enabled.
> 
> Cc: Will Deacon 
> Cc: Dave Martin 
> Signed-off-by: Suzuki K Poulose 
> ---
>  arch/arm64/include/asm/cpufeature.h |  9 +
>  arch/arm64/kernel/cpufeature.c  | 11 ++-
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h 
> b/arch/arm64/include/asm/cpufeature.h
> index 7bb3fdec827e..71993dd4afae 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -223,6 +223,15 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU |   \
>ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU)
>  
> +/*
> + * CPU feature detected at boot time, on one or more CPUs. A late CPU
> + * is not allowed to have the capability when the system doesn't have it.
> + * It is Ok for a late CPU to miss the feature.
> + */
> +#define ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE   \
> + (ARM64_CPUCAP_SCOPE_LOCAL_CPU   |   \
> +  ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU)
> +
>  struct arm64_cpu_capabilities {
>   const char *desc;
>   u16 capability;
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index ecc87aa74c64..4a55492784b7 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -862,9 +862,8 @@ static bool has_no_fpsimd(const struct 
> arm64_cpu_capabilities *entry, int __unus
>  static int __kpti_forced; /* 0: not forced, >0: forced on, <0: forced off */
>  
>  static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
> - int __unused)
> + int scope)
>  {
> - u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
>  
>   /* Forced on command line? */
>   if (__kpti_forced) {
> @@ -885,8 +884,7 @@ static bool unmap_kernel_at_el0(const struct 
> arm64_cpu_capabilities *entry,
>   }
>  
>   /* Defer to CPU feature registers */
> - return !cpuid_feature_extract_unsigned_field(pfr0,
> -  ID_AA64PFR0_CSV3_SHIFT);
> + return !has_cpuid_feature(entry, scope);
>  }
>  
>  static int __init parse_kpti(char *str)
> @@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
> arm64_features[] = {
>   {
>   .desc = "Kernel page table isolation (KPTI)",
>   .capability = ARM64_UNMAP_KERNEL_AT_EL0,
> - .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> + .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
> + .sys_reg = SYS_ID_AA64PFR0_EL1,
> + .field_pos = ID_AA64PFR0_CSV3_SHIFT,
> + .min_field_value = 1,
>   .matches = unmap_kernel_at_el0,

Minor nit, but:

Can we have a comment here to explain that .min_field_value is the
minimum value that indicates that KPTI is _not_ required by this cpu?
This is the opposite of the usual semantics for this field.

Otherwise, this inversion of meaning is not obvious without digging into
unmap_kernel_at_el0() and spotting the ! in !has_cpuid_feature().

With that, or if this usage of !has_cpuid_feature() is already well-
established so that a comment is deemed unnecessary:

Reviewed-by: Dave Martin 

Cheers
---Dave

[PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-01-31 Thread Suzuki K Poulose 
KPTI is treated as a system wide feature, where we enable the feature
when all the CPUs on the system suffers from the security vulnerability,
unless it is forced via kernel command line. Also, if a late CPU needs
KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
to boot, which is a potential security vulnerability.  This patch ensures
that late CPUs are rejected as appropriate if they need KPTI but it wasn't
enabled.

Cc: Will Deacon 
Cc: Dave Martin 
Signed-off-by: Suzuki K Poulose 
---
 arch/arm64/include/asm/cpufeature.h |  9 +
 arch/arm64/kernel/cpufeature.c  | 11 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 7bb3fdec827e..71993dd4afae 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -223,6 +223,15 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
 ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU |   \
 ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU)
 
+/*
+ * CPU feature detected at boot time, on one or more CPUs. A late CPU
+ * is not allowed to have the capability when the system doesn't have it.
+ * It is Ok for a late CPU to miss the feature.
+ */
+#define ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE \
+   (ARM64_CPUCAP_SCOPE_LOCAL_CPU   |   \
+ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU)
+
 struct arm64_cpu_capabilities {
const char *desc;
u16 capability;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index ecc87aa74c64..4a55492784b7 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -862,9 +862,8 @@ static bool has_no_fpsimd(const struct 
arm64_cpu_capabilities *entry, int __unus
 static int __kpti_forced; /* 0: not forced, >0: forced on, <0: forced off */
 
 static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
-   int __unused)
+   int scope)
 {
-   u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
 
/* Forced on command line? */
if (__kpti_forced) {
@@ -885,8 +884,7 @@ static bool unmap_kernel_at_el0(const struct 
arm64_cpu_capabilities *entry,
}
 
/* Defer to CPU feature registers */
-   return !cpuid_feature_extract_unsigned_field(pfr0,
-ID_AA64PFR0_CSV3_SHIFT);
+   return !has_cpuid_feature(entry, scope);
 }
 
 static int __init parse_kpti(char *str)
@@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
arm64_features[] = {
{
.desc = "Kernel page table isolation (KPTI)",
.capability = ARM64_UNMAP_KERNEL_AT_EL0,
-   .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+   .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
+   .sys_reg = SYS_ID_AA64PFR0_EL1,
+   .field_pos = ID_AA64PFR0_CSV3_SHIFT,
+   .min_field_value = 1,
.matches = unmap_kernel_at_el0,
},
 #endif
-- 
2.14.3

[PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs

2018-01-31 Thread Suzuki K Poulose 
KPTI is treated as a system wide feature, where we enable the feature
when all the CPUs on the system suffers from the security vulnerability,
unless it is forced via kernel command line. Also, if a late CPU needs
KPTI but KPTI was not enabled at boot time, the CPU is currently allowed
to boot, which is a potential security vulnerability.  This patch ensures
that late CPUs are rejected as appropriate if they need KPTI but it wasn't
enabled.

Cc: Will Deacon 
Cc: Dave Martin 
Signed-off-by: Suzuki K Poulose 
---
 arch/arm64/include/asm/cpufeature.h |  9 +
 arch/arm64/kernel/cpufeature.c  | 11 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 7bb3fdec827e..71993dd4afae 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -223,6 +223,15 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
 ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU |   \
 ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU)
 
+/*
+ * CPU feature detected at boot time, on one or more CPUs. A late CPU
+ * is not allowed to have the capability when the system doesn't have it.
+ * It is Ok for a late CPU to miss the feature.
+ */
+#define ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE \
+   (ARM64_CPUCAP_SCOPE_LOCAL_CPU   |   \
+ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU)
+
 struct arm64_cpu_capabilities {
const char *desc;
u16 capability;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index ecc87aa74c64..4a55492784b7 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -862,9 +862,8 @@ static bool has_no_fpsimd(const struct 
arm64_cpu_capabilities *entry, int __unus
 static int __kpti_forced; /* 0: not forced, >0: forced on, <0: forced off */
 
 static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
-   int __unused)
+   int scope)
 {
-   u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
 
/* Forced on command line? */
if (__kpti_forced) {
@@ -885,8 +884,7 @@ static bool unmap_kernel_at_el0(const struct 
arm64_cpu_capabilities *entry,
}
 
/* Defer to CPU feature registers */
-   return !cpuid_feature_extract_unsigned_field(pfr0,
-ID_AA64PFR0_CSV3_SHIFT);
+   return !has_cpuid_feature(entry, scope);
 }
 
 static int __init parse_kpti(char *str)
@@ -1008,7 +1006,10 @@ static const struct arm64_cpu_capabilities 
arm64_features[] = {
{
.desc = "Kernel page table isolation (KPTI)",
.capability = ARM64_UNMAP_KERNEL_AT_EL0,
-   .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+   .type = ARM64_CPUCAP_BOOT_RESTRICTED_CPU_LOCAL_FEATURE,
+   .sys_reg = SYS_ID_AA64PFR0_EL1,
+   .field_pos = ID_AA64PFR0_CSV3_SHIFT,
+   .min_field_value = 1,
.matches = unmap_kernel_at_el0,
},
 #endif
-- 
2.14.3