Re: [RFC PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN

From: Vladimir Murzin <vladimir.murzin@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: keescook@chromium.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN
Date: Wed, 18 Nov 2020 12:37:40 +0000	[thread overview]
Message-ID: <5e4cdc4a-d8be-6df7-e096-018cc3fe3463@arm.com> (raw)
In-Reply-To: <X7P+r/l3ewvaf1aV@trantor>

On 11/17/20 4:48 PM, Catalin Marinas wrote:
> On Fri, Nov 13, 2020 at 03:20:22PM +0000, Vladimir Murzin wrote:
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 4ff12a7..d1f68d2 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -113,8 +113,15 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
>>  #define pte_dirty(pte)		(pte_sw_dirty(pte) || pte_hw_dirty(pte))
>>  
>>  #define pte_valid(pte)		(!!(pte_val(pte) & PTE_VALID))
>> -#define pte_valid_not_user(pte) \
>> -	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
>> +#define pte_valid_not_user(pte)										\
>> +({													\
>> +	int __val;											\
>> +	if (cpus_have_const_cap(ARM64_HAS_EPAN))							\
>> +		__val = (pte_val(pte) & (PTE_VALID | PTE_USER | PTE_UXN)) == (PTE_VALID | PTE_UXN);	\
>> +	else												\
>> +		__val = (pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID;				\
>> +	__val;												\
> 
> Is it worth having the cap check here? I'd go with the PTE_VALID|PTE_UXN
> check only.
> 

I do not know to be honest. I do not have full picture in mind and what could be side effects of the
change (that's why RFC). 24cecc377463 the PTE_VALID|PTE_UXN moved to PTE_VALID, so I decided to be
safe than sorry...

>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index dcc165b..2033e0b 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1602,6 +1602,13 @@ static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
>>  }
>>  #endif /* CONFIG_ARM64_PAN */
>>  
>> +#ifdef CONFIG_ARM64_EPAN
>> +static void cpu_enable_epan(const struct arm64_cpu_capabilities *__unused)
>> +{
>> +	sysreg_clear_set(sctlr_el1, 0, SCTLR_EL1_EPAN);
>> +}
>> +#endif /* CONFIG_ARM64_EPAN */
> 
> I checked the spec (2020 arch updates) and the EPAN bit is permitted to
> be cached in the TLB. I think we get away with this because this
> function is called before cnp is enabled. Maybe we should make it
> explicit and move the CnP entry last with a comment.
> 

Hmm, so we rely on CnP's enable method to (indirectly) involve local_flush_tlb_all()? It doesn't seem
robust since CONFIG_ARM64_CNP could be unset. I can add local_flush_tlb_all() into cpu_enable_epan()
or we can have something like

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index bb2016c..0f0a27b 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2416,6 +2416,8 @@ static int cpu_enable_non_boot_scope_capabilities(void *__unused)
                if (cap->cpu_enable)
                        cap->cpu_enable(cap);
        }
+
+       local_flush_tlb_all();
        return 0;
 }
 
@@ -2467,6 +2469,8 @@ static void __init enable_cpu_capabilities(u16 scope_mask)
        if (!boot_scope)
                stop_machine(cpu_enable_non_boot_scope_capabilities,
                             NULL, cpu_online_mask);
+       else
+               local_flush_tlb_all();
 }
 
 /*

What would be your preference?

Cheers
Vladimir

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
