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

[Catalin Marinas <catalin.marinas@arm.com>]

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.

-- 
Catalin

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