Re: [RFC PATCH v2 13/15] arm64: mm: Guard page table writes with kpkeys

[Kevin Brodsky <kevin.brodsky@arm.com>]

On 09/01/2025 08:17, Qi Zheng wrote:
> [...]
>
>> @@ -314,6 +315,7 @@ static inline pte_t pte_clear_uffd_wp(pte_t pte)
>>     static inline void __set_pte_nosync(pte_t *ptep, pte_t pte)
>>   {
>> +    guard(kpkeys_hardened_pgtables)();
>>       WRITE_ONCE(*ptep, pte);
>>   }
>>   @@ -758,6 +760,7 @@ static inline void set_pmd(pmd_t *pmdp, pmd_t pmd)
>>       }
>>   #endif /* __PAGETABLE_PMD_FOLDED */
>>   +    guard(kpkeys_hardened_pgtables)();
>>       WRITE_ONCE(*pmdp, pmd);
>>         if (pmd_valid(pmd)) {
>
> I noticed a long time ago that set_pte/set_pmd/... was implemented
> separately by each architecture without a unified entry point. This
> makes it difficult to add some hooks for them.
>
> Taking set_pte() as an example, is it possible to do the following:
>
> 1) add a generic set_pte() in include/asm-generic/tlb.h (Or other more
>    appropriate files)
>
> static inline void set_pte(pte_t *ptep, pte_t pte)
> {
>     arch_set_pte(ptep, pte);
> }
>
> 2) let each architecture include this file and rename the original
>    set_pte() to arch_set_pte().
>
> 3) then we can add hooks for generic set_pte():
>
> static inline void set_pte(pte_t *ptep, pte_t pte)
> {
>     guard(kpkeys_hardened_pgtables)();
>     arch_set_pte(ptep, pte);
> }
>
> 4) in this way, the architecture that supports
>    ARCH_HAS_KPKEYS_HARDENED_PGTABLES only needs to implement
>    the kpkeys_hardened_pgtables(), otherwise it is no-op.

Thanks for chiming in, it is an interesting idea for sure. I think the
issue here might be the benefit/churn ratio, because this would simply
be adding a layer of (generic) function calls without unifying any
existing arch code. Unfortunately, unlike the pagetable_alloc/tlb stuff,
the majority of what happens in the page table modifiers is arch-specific.

For set_p*(), we could potentially have it call a generic __set_p*()
that does a WRITE_ONCE(), which is what most architectures do (in
addition to various other things, like DSB/ISB on arm64). Adding the
kpkeys_hardened_pgtables guard there would be better, as it reduces the
"privileged" window to just the write itself. However for the other
modifiers, say ptep_get_and_clear(), the implementation seems to vary
wildly between arch's, including the atomic operation itself. Any
unification of those seems therefore difficult.

That said, I'd be happy to look into adding that generic layer on top
(i.e. generic set_pte() calls arch_set_pte()) if there is enough
consensus that the churn is justified. We could potentially do a mix of
both as well (arch-defined set_pte() calls generic __set_pte(), while
generic ptep_get_and_clear() calls arch_ptep_get_and_clear()).

- Kevin