[linux-morello] Re: [PATCH v1 2/2] arm64: rewrite Morello helpers in C

On 22/02/2024 12:15, Akram Ahmad wrote:

Many of the Morello helper functions are implemented in assembly due to historical reasons. This patch rewrites all of the helpers defined in morello.S into C to improve their readability. This patch also removes the original morello.S assembly file and edits the Makefile accordingly.

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

arch/arm64/include/asm/assembler.h | 10 -- arch/arm64/include/asm/morello.h | 4 + arch/arm64/kernel/asm-offsets.c | 8 -- arch/arm64/kernel/morello.c | 125 +++++++++++++++++- arch/arm64/lib/Makefile | 3 +- arch/arm64/lib/morello.S | 205 ----------------------------- 6 files changed, 123 insertions(+), 232 deletions(-) delete mode 100644 arch/arm64/lib/morello.S

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 7d4046ded4df..11596bee98fe 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -597,16 +597,6 @@ alternative_endif mrs \rd, sp_el0 .endm -/*

• • Return a pointer to tsk's pt_regs.
• */
• .macro get_task_pt_regs, rd:req, tsk:req
• /* See task_pt_regs() in processor.h */
• ldr \rd, [\tsk, #TSK_STACK]
• add \rd, \rd, #THREAD_SIZE
• sub \rd, \rd, #PT_REGS_SIZE
• .endm

/*

• Offset ttbr1 to allow for 48-bit kernel VAs set with 52-bit PTRS_PER_PGD.
• orr is used as it can cover the immediate value (and is idempotent).

diff --git a/arch/arm64/include/asm/morello.h b/arch/arm64/include/asm/morello.h index 357f403d073d..be76926e5d0f 100644 --- a/arch/arm64/include/asm/morello.h +++ b/arch/arm64/include/asm/morello.h @@ -5,10 +5,14 @@ #ifndef __ASM_MORELLO_H #define __ASM_MORELLO_H +#ifdef __ASSEMBLY__

The #ifndef __ASSEMBLY__ below is followed by an #else section, so it would be better to move the #define's there instead (avoids adding a new #ifdef).

/* Architectural definitions */ #define MORELLO_CAP_PERM_EXECUTIVE_BIT 1 #define MORELLO_CAP_PERM_EXECUTIVE_MASK (1 << MORELLO_CAP_PERM_EXECUTIVE_BIT) +#endif

#ifndef __ASSEMBLY__ struct pt_regs; diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index 90ca25039583..defc1f6f5d11 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -55,14 +55,6 @@ int main(void) #endif #ifdef CONFIG_ARM64_MTE DEFINE(THREAD_MTE_CTRL, offsetof(struct task_struct, thread.mte_ctrl)); -#endif -#ifdef CONFIG_ARM64_MORELLO

• DEFINE(THREAD_MORELLO_USER_STATE, offsetof(struct task_struct, thread.morello_user_state));
• BLANK();
• DEFINE(MORELLO_STATE_CTPIDR, offsetof(struct morello_state, ctpidr));
• DEFINE(MORELLO_STATE_DDC, offsetof(struct morello_state, ddc));
• DEFINE(MORELLO_STATE_CID, offsetof(struct morello_state, cid));
• DEFINE(MORELLO_STATE_CCTLR, offsetof(struct morello_state, cctlr));

#endif BLANK(); DEFINE(S_X0, offsetof(struct pt_regs, regs[0])); diff --git a/arch/arm64/kernel/morello.c b/arch/arm64/kernel/morello.c index 7dfbca70e11d..8108e6256de2 100644 --- a/arch/arm64/kernel/morello.c +++ b/arch/arm64/kernel/morello.c @@ -20,12 +20,24 @@ #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,

• 	     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); +static void __morello_cap_lo_hi_tag(uintcap_t cap, u64 *lo_val, u64 *hi_val,

• 	     u8 *tag)
  

+{

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

No need to cast to u64, this is the type that copy_from_high returns.

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

No need for the cast here.

+}

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

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

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

+}

+static bool __morello_cap_has_executive(uintcap_t cap) +{

• return cheri_perms_get(cap) & ARM_CAP_PERMISSION_EXECUTIVE;

+}

These functions should be moved with the other static functions. Generally speaking functions are defined last in a .c file (after #include's, constants, globals). The declarations were a bit of a special case (they normally belong to a header, but notionally they were private to this file).

We could also drop the __morello prefix, which was used to make clear the assembly functions were private. The existing static functions have no prefix, e.g. is_pure_task().

static uintcap_t morello_sentry_unsealcap __ro_after_init; @@ -53,6 +65,26 @@ static void update_regs_c64(struct pt_regs *regs, unsigned long pc) } } +void morello_cap_get_val_tag(uintcap_t cap, __uint128_t *val, u8 *tag) +{

• *((uintcap_t *)val) = cheri_tag_clear(cap);

Nit: two spaces between uintcap_t and *.

• *tag = cheri_tag_get(cap);

+}

+uintcap_t morello_build_any_user_cap(const __uint128_t *val, u8 tag) +{

• uintcap_t cap = *((uintcap_t *)val);
• if (!tag)

• return cheri_tag_clear(cap);
  

• uintcap_t sealing_cap = cheri_type_copy(cheri_user_root_allperms_cap, cap);
• cap = (user_uintptr_t)cheri_cap_build(cap, cheri_user_root_allperms_cap);

Arguments to cheri_cap_build() are reversed compared to the BUILD instruction. Unfortunately that's pretty hard to figure out, the safest is to check with Compiler Explorer [1].

[1] https://cheri-compiler-explorer.cl.cam.ac.uk/

• cap = cheri_seal_conditionally(cap, sealing_cap);
• return cap;

+}

void morello_thread_start(struct pt_regs *regs, unsigned long pc) { update_regs_c64(regs, pc); @@ -79,7 +111,86 @@ void morello_thread_init_user(void) uintcap_t ddc = is_pure_task() ? cheri_user_root_cap : cheri_user_root_allperms_cap;

Nit: generally no empty line between variable declarations.

• __morello_thread_init_user(current, ddc);

• struct morello_state *morello_state = &current->thread.morello_user_state;
• /*

• * 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.
  

It's not clear why, because this is no longer preceded by the comment about CTPIDR. Is there a particular reason not to keep it?

• * 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.
  

• */
  

• clear_cap_sysreg(rctpidr_el0);
• write_cap_sysreg(ddc, ddc_el0);
• clear_cap_sysreg(rddc_el0);
• morello_state->ddc = ddc;
• morello_state->rddc = (uintcap_t)0;

There's no cast in the same situation two lines below. No strong preference but it would be better to do the same in both cases.

Also maybe add an empty line here to make the grouping clearer, and between CID and CCTLR too.

• clear_cap_sysreg(cid_el0);
• morello_state->cid = 0;
• write_sysreg(0, cctlr_el0);
• morello_state->cctlr = 0;

+}

+void morello_thread_save_user_state(struct task_struct *tsk) +{

• struct morello_state *morello_state = &tsk->thread.morello_user_state;
• /* (R)CTPIDR is handled by task_save_user_tls */
• morello_state->ddc = read_cap_sysreg(ddc_el0);
• morello_state->rddc = read_cap_sysreg(rddc_el0);
• morello_state->cid = read_cap_sysreg(cid_el0);
• morello_state->cctlr = read_sysreg(cctlr_el0);

+}

+void morello_thread_restore_user_state(struct task_struct *tsk) +{

• struct morello_state *morello_state = &tsk->thread.morello_user_state;
• /* (R)CTPIDR is handled by task_restore_user_tls */
• write_cap_sysreg(morello_state->ddc, ddc_el0);
• write_cap_sysreg(morello_state->rddc, rddc_el0);
• write_cap_sysreg(morello_state->cid, cid_el0);
• write_sysreg(morello_state->cctlr, cctlr_el0);

+}

+void morello_task_save_user_tls(struct task_struct *tsk, user_uintptr_t *tp_ptr) +{

• struct morello_state *morello_state = &tsk->thread.morello_user_state;
• struct pt_regs *regs = task_pt_regs(tsk);
• uintcap_t active_ctpidr;
• morello_state->ctpidr = read_cap_sysreg(ctpidr_el0);
• morello_state->rctpidr = read_cap_sysreg(rctpidr_el0);
• if (__morello_cap_has_executive(regs->pcc))

• active_ctpidr = read_cap_sysreg(ctpidr_el0);
  

Duplicating calls to read*_sysreg should be avoided. Because they're implemented as asm volatile, they cannot be optimised out by the compiler (i.e. the register will be read every time the function is called). On the other hand, morello_state is a regular pointer, so the compiler can assume that morello_state->ctpidr doesn't change, and it doesn't need to be loaded again if we refer to it here.

• else

• active_ctpidr = read_cap_sysreg(rctpidr_el0);
  

• *tp_ptr = (user_uintptr_t)active_ctpidr;

+}

+void morello_task_restore_user_tls(struct task_struct *tsk,

• 		   const user_uintptr_t *tp_ptr)
  

+{

• struct morello_state *morello_state = &tsk->thread.morello_user_state;
• struct pt_regs *regs = task_pt_regs(tsk);
• uintcap_t *active_ctpidr;
• if (__morello_cap_has_executive(regs->pcc))

• active_ctpidr = &morello_state->ctpidr;
  

• else

• active_ctpidr = &morello_state->rctpidr;
  

+#ifdef CONFIG_CHERI_PURECAP_UABI

• *active_ctpidr = (uintcap_t)*tp_ptr;

+#else

• __morello_merge_c_x(active_ctpidr, (u64)*tp_ptr);

Given the #ifdef, we can drop the casts in both cases here (user_uintptr_t is uintcap_t in PCuABI by definition, and u64 otherwise).

Kevin

+#endif

• write_cap_sysreg(morello_state->ctpidr, ctpidr_el0);
• write_cap_sysreg(morello_state->rctpidr, rctpidr_el0);

} [...]