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

[Vladimir Murzin <vladimir.murzin@arm.com>]

On 11/18/20 4:04 PM, Catalin Marinas wrote:
> On Wed, Nov 18, 2020 at 12:37:40PM +0000, Vladimir Murzin wrote:
>> 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...
> 
> A user has access to a page if it has PTE_VALID && (PTE_USER || !PTE_UXN)
> (wrong user of the logic operators but you get the idea). So negating
> the user part in the above expression, pte_valid_not_user() means
> PTE_VALID && !PTE_USER && PTE_UXN.
> 
> Prior to these patches (or the old exec-only), we can't have PTE_UXN and
> PTE_USER both cleared, this is introduced by PAGE_EXECONLY. IOW, without
> EPAN, !PTE_USER implies PTE_UXN, so we can use the same check as for the
> EPAN case.
> 
>>>> 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
> 
> A local_flush_tlb_all() in cpu_enable_epan() would be fine before user
> space starts. However, a late CPU bring-up may cause a temporary
> disabling of EPAN in the sibling core if CnP is enabled first.
> 
>> 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();
>>  }
> 
> Any local TLBI would clear the mismatch but it doesn't solve the
> temporary difference between sibling cores. I think the only guarantee
> here is if CnP is turned on after the feature in question.
> 

Thanks for explanation, I'll send updated version shortly!

Cheers
Vladimir

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