On Wed, Feb 07, 2018 at 06:15:58PM +0000, Suzuki K Poulose wrote:
> On 07/02/18 10:38, Dave Martin wrote:
> >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"?
> >
>
> 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
