Messages in this thread Patch in this message | | | Date | Wed, 7 Feb 2018 10:38:45 +0000 | From | Dave Martin <> | Subject | Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to boot-time CPUs |
| |
On Wed, Jan 31, 2018 at 06:27:57PM +0000, 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 <will.deacon@arm.com> > Cc: Dave Martin <dave.martin@arm.com> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > 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 <Dave.Martin@arm.com>
Cheers ---Dave
| |