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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ec712e11-baa5-aa2c-1b70-be221ea36f9f@arm.com>
Date:   Wed, 7 Feb 2018 18:15:58 +0000
From:   Suzuki K Poulose <Suzuki.Poulose@....com>
To:     Dave Martin <Dave.Martin@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, mark.rutland@....com,
        ckadabi@...eaurora.org, ard.biesheuvel@...aro.org,
        marc.zyngier@....com, catalin.marinas@....com, will.deacon@....com,
        linux-kernel@...r.kernel.org, jnair@...iumnetworks.com
Subject: Re: [PATCH v2 10/20] arm64: capabilities: Restrict KPTI detection to
 boot-time CPUs

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..."
.
>> that late CPUs are rejected as appropriate if they need KPTI but it wasn't
>> enabled.
>>
>> Cc: Will Deacon <will.deacon@....com>
>> Cc: Dave Martin <dave.martin@....com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....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.

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