[linux-morello] Re: [PATCH v0 1/3] arm64: rewrite Morello helpers in C

On 12/02/2024 16:54, Akram Ahmad wrote:

Many of the Morello helper functions are implemented in assembly due to historical reasons. This patch rewrites the following helpers in C to improve their readability:

• __morello_cap_lo_hi_tag
• __morello_cap_has_executive
• __morello_merge_c_x
• __morello_thread_init_user

The split between patch 1 and 3 currently seems to be that patch 1 takes care of private helpers and patch 3 of the rest. It makes some sense, but it doesn't improve readability much compared to just one patch. A better logical split might be between general helpers and those that context-switch / initialise morello_user_state, since the latter are pretty similar to one another (and the most complex).

Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com

arch/arm64/kernel/morello.c | 44 ++++++++++++++++++++++++---- arch/arm64/lib/morello.S | 58 ------------------------------------- 2 files changed, 39 insertions(+), 63 deletions(-)

diff --git a/arch/arm64/kernel/morello.c b/arch/arm64/kernel/morello.c index e3f732f4c52d..644edfd44b46 100644 --- a/arch/arm64/kernel/morello.c +++ b/arch/arm64/kernel/morello.c @@ -20,12 +20,46 @@ #include <asm/morello.h> #include <asm/ptrace.h> -/* Private functions implemented in morello.S */ void __morello_cap_lo_hi_tag(uintcap_t cap, u64 *lo_val, u64 *hi_val,

Helpers that are only used inside morello.c should now be static. Until now they were implemented in a different translation unit so they had to be declared global.

• 	     u8 *tag);
  

-void __morello_merge_c_x(uintcap_t *creg, u64 xreg); -bool __morello_cap_has_executive(uintcap_t cap); -void __morello_thread_init_user(struct task_struct *tsk, uintcap_t ddc);

• 	     u8 *tag)
  

+{

• *lo_val = cap;

Let's use an explicit cast to u64 for clarity.

• *hi_val = __builtin_cheri_copy_from_high((void __capability *)cap);

The CHERI builtins are normally polymorphic: they accept both void * __capability and uintcap_t, and those returning a capability (e.g. cheri_address_set()) return the same type they are passed. It looks like this particular one still only accepts void * __capability (I've reported that), but it should be possible to remove the casts when calling the other builtins.

Another note: when using __capability explicitly, always use it in postfix position: void * __capability, and not void __capability *. The latter is deprecated and ambiguous in complex cases, it just happens that we need it for __user.

• *tag = cheri_tag_get((void __capability *)&cap);

&cap is a (64-bit) pointer to cap, which will end up being stored on the stack. Casting it to void * __capability creates a capability out of it, derived from the kernel's DDC. Probably not what you meant to do :)

+}

+void __morello_merge_c_x(uintcap_t *creg, u64 xreg) +{

• if (cheri_address_get(*creg) != xreg)

• *creg = (uintcap_t)cheri_address_set(*creg, xreg);
  

No need for a cast when assigning either, as the polymorphic builtins return the same type they are passed (uintcap_t here).

+}

+bool __morello_cap_has_executive(uintcap_t cap) +{

• return cheri_perms_get((void __capability *)cap) & MORELLO_CAP_PERM_EXECUTIVE_MASK;

Let's use the standard permission values from cheriintrin.h, ARM_CAP_PERMISSION_EXECUTIVE here. We still need MORELLO_CAP_PERM_EXECUTIVE_MASK for the assembly macros, but we could move its definition inside the #ifdef __ASSEMBLY__  in morello.h to avoid confusion.

+}

+void __morello_thread_init_user(struct task_struct *tsk, uintcap_t ddc)

This function exists because morello_thread_init_user() needed an assembly helper. Now that we're writing everything in C, we don't need it any more - just fold into morello_thread_init_user().

+{

• tsk->thread.morello_user_state.ddc = ddc;
• tsk->thread.morello_user_state.rddc = (uintcap_t)as_user_ptr(0);

(uintcap_t)0 is good enough, no need to use user pointer helpers here.

• tsk->thread.morello_user_state.cid = (uintcap_t)as_user_ptr(0);
• tsk->thread.morello_user_state.cctlr = 0;

Nit: for all these functions that access a bunch of fields in morello_user_state, it could be a bit more readable with a local pointer:

struct morello_state *morello_state = &tsk->thread.morello_user_state;

That would also be consistent with morello_flush_cap_regs_to_64_regs().

• /*

• * tls_thread_flush() does not touch rctpidr_el0 so this must be zeroed
  

• * here. We do not need to initialise its value in morello_user_state.
  

• * Only the ddc_el0 register must be initialised to the specific value;
  

• * RDDC is set to a null capability as processes are always started in
  

• * Executive.
  

• */
  

• asm("msr rctpidr_el0, czr\n\t"

•    "msr ddc_el0, %0\n\t"
  

•    "msr rddc_el0, czr\n\t"
  

•    "msr cid_el0, czr\n\t"
  

•    "msr cctlr_el0, xzr"
  

•    : /* outputs */ :/* inputs */ "r"(ddc) : /*clobbers */);
  

Nit: I'd rather keep the same order as in the assembly (i.e. intermixing setting registers and morello_user_state).

Kevin

+} static uintcap_t morello_sentry_unsealcap __ro_after_init; diff --git a/arch/arm64/lib/morello.S b/arch/arm64/lib/morello.S index 0800375ef33d..5a4a39badfe2 100644 --- a/arch/arm64/lib/morello.S +++ b/arch/arm64/lib/morello.S @@ -46,38 +46,6 @@ SYM_FUNC_START(morello_build_any_user_cap) ret SYM_FUNC_END(morello_build_any_user_cap) -SYM_FUNC_START(__morello_thread_init_user)

• mov x9, #THREAD_MORELLO_USER_STATE
• add x0, x0, x9 // x0 = tsk->thread.morello_user_state
• /*

• * CTPIDR doesn't need to be initialised explicitly:
  

• * - tls_thread_flush() already zeroes tpidr_el0, zeroing ctpidr_el0 as
  

• *   well
  

• * - The value stored in thread.morello_user_state will be set the next
  

• *   time task_save_user_tls() is called, like thread_struct.uw.tp_value.
  

• *
  

• * tls_thread_flush() does not touch rcsp_el0, so we need to zero it
  

• * here, but its value in morello_user_state does not need to be
  

• * initialised here either.
  

• */
  

• msr rctpidr_el0, czr
• /* DDC: initialised to the specified value */
• msr ddc_el0, c1
• /* RDDC: null capability (processes are always started in Executive) */
• msr rddc_el0, czr
• stp c1, czr, [x0, #MORELLO_STATE_DDC]
• /* CID: null capability */
• msr cid_el0, czr
• str czr, [x0, #MORELLO_STATE_CID]
• /* CCTLR: all bits cleared */
• msr cctlr_el0, xzr
• str xzr, [x0, #MORELLO_STATE_CCTLR]
• ret

-SYM_FUNC_END(__morello_thread_init_user)

SYM_FUNC_START(morello_thread_save_user_state) mov x9, #THREAD_MORELLO_USER_STATE add x0, x0, x9 // x0 = tsk->thread.morello_user_state @@ -177,29 +145,3 @@ SYM_FUNC_START(morello_task_restore_user_tls) ret SYM_FUNC_END(morello_task_restore_user_tls)

-SYM_FUNC_START(__morello_cap_lo_hi_tag)

• str x0, [x1]
• cfhi x4, c0 // Extract upper 64 bits
• str x4, [x2]
• gctag x4, c0
• strb w4, [x3]
• ret

-SYM_FUNC_END(__morello_cap_lo_hi_tag)

-SYM_FUNC_START(__morello_merge_c_x)

• ldr c2, [x0]
• cmp x2, x1
• b.eq 1f
• scvalue c2, c2, x1
• str c2, [x0]

-1:

• ret

-SYM_FUNC_END(__morello_merge_c_x)

-SYM_FUNC_START(__morello_cap_has_executive)

• gcperm x0, c0
• ubfx x0, x0, #MORELLO_CAP_PERM_EXECUTIVE_BIT, #1
• ret

-SYM_FUNC_END(__morello_cap_has_executive)