On 02/09/2022 11:45, Sherwin da Cruz wrote:

To aid in libc development, additional auxiliary vector entries are added in the native elf loader. This is supplemented by code that determines the beginning and end addresses of the load segments in the elf and interpreter. A full description of the entries and their constant values can be found in the PCuABI specification at [1].

Nit: since you mention both the entry description and constant values, I'd leave out the section reference in the URL (also because the section name could potentially change).

Signed-off-by: Sherwin da Cruz sherwin.dacruz@arm.com

fs/binfmt_elf.c | 99 +++++++++++++++++++++++++++++++++++-- include/linux/auxvec.h | 2 +- include/uapi/linux/auxvec.h | 13 +++++ 3 files changed, 109 insertions(+), 5 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index e9e6ab6570bc..dcbb2fc83f14 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -49,6 +49,10 @@ #include <asm/param.h> #include <asm/page.h> +#ifdef CONFIG_CHERI_PURECAP_UABI +#include <cheriintrin.h> +#endif

• #ifndef ELF_COMPAT #define ELF_COMPAT 0 #endif

@@ -177,10 +181,19 @@ static int padzero(unsigned long elf_bss) #define ELF_BASE_PLATFORM NULL #endif +struct elf_load_info {

• unsigned long start_elf_rx;
• unsigned long end_elf_rx;
• unsigned long start_elf_rw;
• unsigned long end_elf_rw;

+};

• static int create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, unsigned long interp_load_addr,

• unsigned long e_entry, unsigned long phdr_addr)
  

• unsigned long e_entry, unsigned long phdr_addr,
  

• const struct elf_load_info *exec_load_info,
  

• const struct elf_load_info *interp_load_info)
  

  { struct mm_struct *mm = current->mm; unsigned long p = bprm->p;

@@ -199,6 +212,9 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, int ei_index; const struct cred *cred = current_cred(); struct vm_area_struct *vma; +#if defined(CONFIG_CHERI_PURECAP_UABI) && (ELF_COMPAT == 0)

• elf_stack_item_t *mm_at_argv, *mm_at_envp;

+#endif /* * In some cases (e.g. Hyper-Threading), we want to avoid L1 @@ -293,6 +309,42 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, if (bprm->have_execfd) { NEW_AUX_ENT(AT_EXECFD, bprm->execfd); } +#if defined(CONFIG_CHERI_PURECAP_UABI) && (ELF_COMPAT == 0)

• /*

• * TODO [PCuABI] - Restrict bounds/perms for AT_CHERI_* entries
  

• */
  

• NEW_AUX_ENT(AT_CHERI_EXEC_RW_CAP,

• (exec_load_info->start_elf_rw != ~0UL ?
  

• 	elf_uaddr_to_user_ptr(exec_load_info->start_elf_rw) :
  

• 	NULL));
  

• NEW_AUX_ENT(AT_CHERI_EXEC_RX_CAP,

• elf_uaddr_to_user_ptr(exec_load_info->start_elf_rx));
  

• NEW_AUX_ENT(AT_CHERI_INTERP_RW_CAP,

• ((interp_load_addr && interp_load_info->start_elf_rw != ~0UL) ?
  

• 	elf_uaddr_to_user_ptr(interp_load_info->start_elf_rw) :
  

• 	NULL));
  

• NEW_AUX_ENT(AT_CHERI_INTERP_RX_CAP,

• (interp_load_addr ?
  

• 	elf_uaddr_to_user_ptr(interp_load_info->start_elf_rx) :
  

• 	NULL));
  

• NEW_AUX_ENT(AT_CHERI_STACK_CAP, elf_uaddr_to_user_ptr(0));
• NEW_AUX_ENT(AT_CHERI_SEAL_CAP,

• cheri_bounds_set_exact(elf_uaddr_to_user_ptr(0), 1 << 15));
  

• NEW_AUX_ENT(AT_CHERI_CID_CAP, elf_uaddr_to_user_ptr(0));
• /*

• * Since the auxv entries are inserted into the mm struct before the
  

• * argv/envp entries are placed on the stack (unknown at this time),
  

• * we save pointers to their positions in the mm struct to update them
  

• * after their positions on the stack are determined.
  

• */
  

• NEW_AUX_ENT(AT_ARGC, argc);
• NEW_AUX_ENT(AT_ARGV, 0);
• mm_at_argv = elf_info - 1;
• NEW_AUX_ENT(AT_ENVC, envc);
• NEW_AUX_ENT(AT_ENVP, 0);
• mm_at_envp = elf_info - 1;

+#endif /* CONFIG_CHERI_PURECAP_UABI && ELF_COMPAT == 0 */ #undef NEW_AUX_ENT /* AT_NULL is zero; clear the rest too */ memset(elf_info, 0, (char *)mm->saved_auxv + @@ -332,6 +384,9 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, return -EFAULT; /* Populate list of argv pointers back to argv strings. */ +#if defined(CONFIG_CHERI_PURECAP_UABI) && (ELF_COMPAT == 0)

• *mm_at_argv = (elf_stack_item_t)sp;

+#endif p = mm->arg_end = mm->arg_start; while (argc-- > 0) { size_t len; @@ -347,6 +402,9 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, mm->arg_end = p; /* Populate list of envp pointers back to envp strings. */ +#if defined(CONFIG_CHERI_PURECAP_UABI) && (ELF_COMPAT == 0)

• *mm_at_envp = (elf_stack_item_t)sp;

+#endif mm->env_end = mm->env_start = p; while (envc-- > 0) { size_t len; @@ -597,7 +655,7 @@ static inline int make_prot(u32 p_flags, struct arch_elf_state *arch_state, static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, struct file *interpreter, unsigned long no_base, struct elf_phdr *interp_elf_phdata,

• struct arch_elf_state *arch_state)
  

• struct arch_elf_state *arch_state, struct elf_load_info *load_info)
  

  { struct elf_phdr *eppnt; unsigned long load_addr = 0;

@@ -625,6 +683,12 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, goto out; }

• load_info->start_elf_rx = ~0UL;
• load_info->start_elf_rw = ~0UL;
• load_info->end_elf_rx = 0;
• load_info->end_elf_rw = 0;
• eppnt = interp_elf_phdata; for (i = 0; i < interp_elf_ex->e_phnum; i++, eppnt++) { if (eppnt->p_type == PT_LOAD) {

@@ -666,6 +730,9 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, error = -ENOMEM; goto out; }

• 	if (eppnt->p_flags & PF_W)
  

• 		load_info->start_elf_rw = min(k, load_info->start_elf_rw);
  

• 	load_info->start_elf_rx = min(k, load_info->end_elf_rx);
  

s/end_elf_rx/start_elf_rx/?

/* * Find the end of the file mapping for this phdr, and @@ -674,6 +741,9 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, k = load_addr + eppnt->p_vaddr + eppnt->p_filesz; if (k > elf_bss) elf_bss = k;

• 	if (eppnt->p_flags & PF_W)
  

• 		load_info->end_elf_rw = max(k, load_info->end_elf_rw);
  

• 	load_info->end_elf_rx = max(k, load_info->end_elf_rx);
  

/* * Do the same thing for the memory mapping - between @@ -848,6 +918,7 @@ static int load_elf_binary(struct linux_binprm *bprm) struct arch_elf_state arch_state = INIT_ARCH_ELF_STATE; struct mm_struct *mm; struct pt_regs *regs;

• struct elf_load_info exec_load_info, interp_load_info;

retval = -ENOEXEC; /* First of all, some simple consistency checks */ @@ -1034,6 +1105,12 @@ static int load_elf_binary(struct linux_binprm *bprm) start_data = 0; end_data = 0;

• exec_load_info.start_elf_rx = ~0UL;
• exec_load_info.start_elf_rw = ~0UL;
• exec_load_info.end_elf_rx = 0;
• exec_load_info.end_elf_rw = 0;
• /* Now we do a little grungy work by mmapping the ELF image into the correct location in memory. */ for(i = 0, elf_ppnt = elf_phdata;

@@ -1200,6 +1277,9 @@ static int load_elf_binary(struct linux_binprm *bprm) start_code = k; if (start_data < k) start_data = k;

• if (elf_ppnt->p_flags & PF_W)
  

• 	exec_load_info.start_elf_rw = min(k, exec_load_info.start_elf_rw);
  

• exec_load_info.start_elf_rx = min(k, exec_load_info.start_elf_rx);
  

/* * Check to see if the section's size will overflow the @@ -1222,6 +1302,10 @@ static int load_elf_binary(struct linux_binprm *bprm) end_code = k; if (end_data < k) end_data = k;

• if (elf_ppnt->p_flags & PF_W)
  

• 	exec_load_info.end_elf_rw = max(k, exec_load_info.end_elf_rw);
  

• exec_load_info.end_elf_rx = max(k, exec_load_info.end_elf_rx);
  

• k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz; if (k > elf_brk) { bss_prot = elf_prot;

@@ -1237,6 +1321,12 @@ static int load_elf_binary(struct linux_binprm *bprm) end_code += load_bias; start_data += load_bias; end_data += load_bias;

• exec_load_info.start_elf_rx += load_bias;
• /* ELF has at least one writable load segment */
• if (exec_load_info.start_elf_rw != ~0UL)

• exec_load_info.start_elf_rw += load_bias;
  

• exec_load_info.end_elf_rx += load_bias;
• exec_load_info.end_elf_rw += load_bias;

This should also be inside the if above to be consistent.

/* Calling set_brk effectively mmaps the pages that we need * for the bss and break sections. We must do this before @@ -1255,7 +1345,7 @@ static int load_elf_binary(struct linux_binprm *bprm) elf_entry = load_elf_interp(interp_elf_ex, interpreter, load_bias, interp_elf_phdata,

• 			    &arch_state);
  

• 			    &arch_state, &interp_load_info);
  

  if (!IS_ERR((void *)elf_entry)) { /* * load_elf_interp() returns relocation

@@ -1295,7 +1385,8 @@ static int load_elf_binary(struct linux_binprm *bprm) #endif /* ARCH_HAS_SETUP_ADDITIONAL_PAGES */ retval = create_elf_tables(bprm, elf_ex, interp_load_addr,

• 		   e_entry, phdr_addr);
  

• 			e_entry, phdr_addr,
  

This alignment change is spurious, it should be left unchanged.

• 			&exec_load_info, &interp_load_info);
  

  if (retval < 0) goto out;

diff --git a/include/linux/auxvec.h b/include/linux/auxvec.h index f68d0ec2d740..41780858473c 100644 --- a/include/linux/auxvec.h +++ b/include/linux/auxvec.h @@ -4,6 +4,6 @@ #include <uapi/linux/auxvec.h> -#define AT_VECTOR_SIZE_BASE 20 /* NEW_AUX_ENT entries in auxiliary table */ +#define AT_VECTOR_SIZE_BASE 31 /* NEW_AUX_ENT entries in auxiliary table */ /* number of "#define AT_.*" above, minus {AT_NULL, AT_IGNORE, AT_NOTELF} */ #endif /* _LINUX_AUXVEC_H */ diff --git a/include/uapi/linux/auxvec.h b/include/uapi/linux/auxvec.h index c7e502bf5a6f..1af95e367b60 100644 --- a/include/uapi/linux/auxvec.h +++ b/include/uapi/linux/auxvec.h @@ -37,4 +37,17 @@ #define AT_MINSIGSTKSZ 51 /* minimal stack size for signal delivery */ #endif +#define AT_CHERI_EXEC_RW_CAP 60 +#define AT_CHERI_EXEC_RX_CAP 61 +#define AT_CHERI_INTERP_RW_CAP 62 +#define AT_CHERI_INTERP_RX_CAP 63 +#define AT_CHERI_STACK_CAP 64 +#define AT_CHERI_SEAL_CAP 65 +#define AT_CHERI_CID_CAP 66

+#define AT_ARGC 80 +#define AT_ARGV 81 +#define AT_ENVC 82 +#define AT_ENVP 83

Let's align the values with tabs (like AT_MINSIGSTKSZ).

Kevin

• #endif /* _UAPI_LINUX_AUXVEC_H */

On 02/09/2022 11:45, Sherwin da Cruz wrote:

The START_THREAD arch hook called in binfmt_elf is modified to return a value which the elf loader returns as the final result. This maintains similar behaviour as before but allows any arch specialising this macro to either 1. Abort execution of an elf binary from within start_thread() by returning a negative value or 2. Place a positive value in the first result register for the executing program to access by returning the positive value.

You are also making another change in this patch, which is that the 4th argument of START_THREAD() is now bprm instead of start_stack, good to call it out here as it's not particularly obvious otherwise. You should also say that this interface change is acceptable because no arch is currently overriding this hook.

Some explanation here about the change to kernel_execve() would be nice too as it's not so obvious why we need it.

Kevin

Signed-off-by: Sherwin da Cruz sherwin.dacruz@arm.com

fs/binfmt_elf.c | 3 +-- fs/compat_binfmt_elf.c | 6 +++++- fs/exec.c | 4 ++-- include/linux/elf.h | 7 +++++-- 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index dcbb2fc83f14..e7f87bf3ff13 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1441,8 +1441,7 @@ static int load_elf_binary(struct linux_binprm *bprm) #endif finalize_exec(bprm);

• START_THREAD(elf_ex, regs, elf_entry, bprm->p);
• retval = 0;

• retval = START_THREAD(elf_ex, regs, elf_entry, bprm); out: return retval;

diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c index 413922b85be6..247b9ebd8fbb 100644 --- a/fs/compat_binfmt_elf.c +++ b/fs/compat_binfmt_elf.c @@ -127,7 +127,11 @@ #ifdef COMPAT_START_THREAD #undef START_THREAD -#define START_THREAD COMPAT_START_THREAD +#define START_THREAD(elf_ex, regs, elf_entry, bprm) \ +({ \

• COMPAT_START_THREAD(elf_ex, regs, elf_entry, bprm->p); \
• 0; /* binfmt_elf return value */ \

+}) #endif #ifdef compat_arch_setup_additional_pages diff --git a/fs/exec.c b/fs/exec.c index c177f8cc3e00..3edd9601ae9a 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1793,7 +1793,7 @@ static int exec_binprm(struct linux_binprm *bprm) trace_sched_process_exec(current, old_pid, bprm); ptrace_event(PTRACE_EVENT_EXEC, old_vpid); proc_exec_connector(current);

• return 0;

• return ret; }

/* @@ -2003,7 +2003,7 @@ int kernel_execve(const char *kernel_filename, free_bprm(bprm); out_ret: putname(filename);

• return retval;

• return retval < 0 ? retval : 0; }

static int do_execve(struct filename *filename, diff --git a/include/linux/elf.h b/include/linux/elf.h index 50ca9e34763c..039ad1867045 100644 --- a/include/linux/elf.h +++ b/include/linux/elf.h @@ -23,8 +23,11 @@ #endif #ifndef START_THREAD -#define START_THREAD(elf_ex, regs, elf_entry, start_stack) \

• start_thread(regs, elf_entry, start_stack)

+#define START_THREAD(elf_ex, regs, elf_entry, bprm) \ +({ \

• start_thread(regs, elf_entry, bprm->p); \
• 0; /* binfmt_elf return value */ \

+}) #endif #if defined(ARCH_HAS_SETUP_ADDITIONAL_PAGES) && !defined(ARCH_SETUP_ADDITIONAL_PAGES)

On 02/09/2022 11:45, Sherwin da Cruz wrote:

In the transitional ABI specific registers are initialised for purecap

This is true in PCuABI in general, the transitional ABI just doesn't specify them so much. Since you don't mention the transitional ABI in patch 2, I think it's fine not to mention it here either (the TODO is clear enough).

applications to make use of. This is set in start_thread(). The macro START_THREAD is specialised to allow returning the value of argc to be placed in x0.

Signed-off-by: Sherwin da Cruz sherwin.dacruz@arm.com

arch/arm64/include/asm/elf.h | 3 +++ arch/arm64/include/asm/processor.h | 30 ++++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h index 9eb86b4cb691..a666e1ccfee1 100644 --- a/arch/arm64/include/asm/elf.h +++ b/arch/arm64/include/asm/elf.h @@ -164,6 +164,9 @@ typedef unsigned long elf_greg_t; typedef elf_greg_t elf_gregset_t[ELF_NGREG]; typedef struct user_fpsimd_state elf_fpregset_t; +#define START_THREAD(elf_ex, regs, elf_entry, bprm) \

You have a mix of tab and spaces aligning , make sure to use just tabs.

• start_thread(regs, elf_entry, bprm->p, bprm->argc, bprm->envc)
• #define SET_PERSONALITY_AARCH64() \ ({ \ current->personality &= ~READ_IMPLIES_EXEC; \

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 5bdecb185f63..88de88ba506e 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -238,6 +238,31 @@ void tls_preserve_current_state(void); .fpsimd_cpu = NR_CPUS, \ } +static inline int init_gp_regs(struct pt_regs *regs, unsigned long sp,

• 				int argc, int envc) {
  

{ is always at the start of a line when defining a function (you can use checkpatch.pl to find about this kind of style guide issues). Also, there are various styles used to indent hanging parameters in the kernel, but other functions in this file align them on the opening parenthesis so you should do the same here.

• int retval = 0;

+#ifdef CONFIG_CHERI_PURECAP_UABI

• unsigned long argv, envp, auxv;
• /*

• * TODO [PCuABI]: When argv/envp/auxv is moved off the stack, update the registers
  

• * x1-x3 with the new pointer values, and c1-c3 with new capabilities to them.
  

Alright so there are two things here really: 1. when the initial args are moved, as you mention, and 2. restricting bounds / permissions. 1. is about x1-x3, and 2. is about c1-c3. The two are not interdependent - you don't have to do both at the same time. Let's make it clear here.

• *
  

• * In ret_to_user c regs are first loaded then merged with x regs if their values
  

• * are different. Hence we load capabilities in c regs and the value in x regs.
  

• */
  

• retval = argc; /* Placed in x0 */
• argv = sp + 1 * sizeof(user_uintptr_t); /* Increment past argc on stack */
• envp = argv + (argc + 1) * sizeof(user_uintptr_t); /* Go past arg vals + NULL */
• auxv = envp + (envc + 1) * sizeof(user_uintptr_t); /* Go past env vals + NULL */
• regs->cregs[1] = regs->cregs[2] = regs->cregs[3] = regs->csp;
• regs->regs[1] = argv;
• regs->regs[2] = envp;
• regs->regs[3] = auxv;

+#endif /* CONFIG_CHERI_PURECAP_UABI */

• return retval;

+}

• static inline void start_thread_common(struct pt_regs *regs, unsigned long pc) { memset(regs, 0, sizeof(*regs));

@@ -248,8 +273,8 @@ static inline void start_thread_common(struct pt_regs *regs, unsigned long pc) regs->pmr_save = GIC_PRIO_IRQON; } -static inline void start_thread(struct pt_regs *regs, unsigned long pc,

• 		unsigned long sp)
  

+static inline int start_thread(struct pt_regs *regs, unsigned long pc,

• 		unsigned long sp, int argc, int envc)
  

  { start_thread_common(regs, pc); regs->pstate = PSR_MODE_EL0t;

@@ -258,6 +283,7 @@ static inline void start_thread(struct pt_regs *regs, unsigned long pc, if (system_supports_morello()) morello_thread_start(regs, pc);

Nit: would look nicer with an empty line here.

Kevin

• return init_gp_regs(regs, sp, argc, envc); }

#ifdef CONFIG_COMPAT

On 05/09/2022 12:33, Sherwin da Cruz wrote:

On 9/2/22 17:07, Kevin Brodsky wrote:

...

On 02/09/2022 11:45, Sherwin da Cruz wrote:

...

In the transitional ABI specific registers are initialised for purecap

This is true in PCuABI in general, the transitional ABI just doesn't specify them so much. Since you don't mention the transitional ABI in patch 2, I think it's fine not to mention it here either (the TODO is clear enough).

...

applications to make use of. This is set in start_thread(). The macro START_THREAD is specialised to allow returning the value of argc to be placed in x0.

Signed-off-by: Sherwin da Cruz sherwin.dacruz@arm.com

arch/arm64/include/asm/elf.h       |  3 +++   arch/arm64/include/asm/processor.h | 30 ++++++++++++++++++++++++++++--   2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h index 9eb86b4cb691..a666e1ccfee1 100644 --- a/arch/arm64/include/asm/elf.h +++ b/arch/arm64/include/asm/elf.h @@ -164,6 +164,9 @@ typedef unsigned long elf_greg_t;   typedef elf_greg_t elf_gregset_t[ELF_NGREG];   typedef struct user_fpsimd_state elf_fpregset_t; +#define START_THREAD(elf_ex, regs, elf_entry, bprm)                    \

You have a mix of tab and spaces aligning , make sure to use just tabs.

...

+    start_thread(regs, elf_entry, bprm->p, bprm->argc, bprm->envc)

#define SET_PERSONALITY_AARCH64()                    \   ({                                    \       current->personality &= ~READ_IMPLIES_EXEC;            \ diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 5bdecb185f63..88de88ba506e 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -238,6 +238,31 @@ void tls_preserve_current_state(void);       .fpsimd_cpu = NR_CPUS,            \   } +static inline int init_gp_regs(struct pt_regs *regs, unsigned long sp, +                        int argc, int envc) {

{ is always at the start of a line when defining a function (you can use checkpatch.pl to find about this kind of style guide issues). Also, there are various styles used to indent hanging parameters in the kernel, but other functions in this file align them on the opening parenthesis so you should do the same here.

...

+    int retval = 0; +#ifdef CONFIG_CHERI_PURECAP_UABI +    unsigned long argv, envp, auxv; +    /* +     * TODO [PCuABI]: When argv/envp/auxv is moved off the stack, update the registers +     * x1-x3 with the new pointer values, and c1-c3 with new capabilities to them.

Alright so there are two things here really: 1. when the initial args are moved, as you mention, and 2. restricting bounds / permissions.

1. is about x1-x3, and 2. is about c1-c3. The two are not

interdependent - you don't have to do both at the same time. Let's make it clear here.

I think I'll specify also that currently c1-c3 is taken from csp, so changing c1-c3 would be needed if csp has already been restricted to the bounds of the stack and the the data has been moved elsewhere.

Right good point, I hadn't noticed you switched to CSP (which makes sense).

Kevin

Sherwin

...

...

+     * +     * In ret_to_user c regs are first loaded then merged with x regs if their values +     * are different. Hence we load capabilities in c regs and the value in x regs. +     */ +    retval = argc; /* Placed in x0 */ +    argv = sp + 1 * sizeof(user_uintptr_t); /* Increment past argc on stack */ +    envp = argv + (argc + 1) * sizeof(user_uintptr_t); /* Go past arg vals + NULL */ +    auxv = envp + (envc + 1) * sizeof(user_uintptr_t); /* Go past env vals + NULL */ +    regs->cregs[1] = regs->cregs[2] = regs->cregs[3] = regs->csp; +    regs->regs[1] = argv; +    regs->regs[2] = envp; +    regs->regs[3] = auxv; +#endif /* CONFIG_CHERI_PURECAP_UABI */

+    return retval; +}

static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)   {       memset(regs, 0, sizeof(*regs)); @@ -248,8 +273,8 @@ static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)           regs->pmr_save = GIC_PRIO_IRQON;   } -static inline void start_thread(struct pt_regs *regs, unsigned long pc, -                unsigned long sp) +static inline int start_thread(struct pt_regs *regs, unsigned long pc, +                unsigned long sp, int argc, int envc)   {       start_thread_common(regs, pc);       regs->pstate = PSR_MODE_EL0t; @@ -258,6 +283,7 @@ static inline void start_thread(struct pt_regs *regs, unsigned long pc,       if (system_supports_morello())           morello_thread_start(regs, pc);

Nit: would look nicer with an empty line here.

Kevin

...

+    return init_gp_regs(regs, sp, argc, envc);   }   #ifdef CONFIG_COMPAT

On 02/09/2022 11:45, Sherwin da Cruz wrote:

Amend an existing morello selftest to check that new capability aux entries are valid and unsealed, as well as check that registers c0-c3 match argc/argv/envp/auxv as viewed on the stack.

Signed-off-by: Sherwin da Cruz sherwin.dacruz@arm.com

.../selftests/arm64/morello/bootstrap.c | 58 ++++++++++++++----- .../arm64/morello/freestanding_start.S | 12 +++- 2 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/arm64/morello/bootstrap.c b/tools/testing/selftests/arm64/morello/bootstrap.c index 92816a82f9e3..b272806a9c2c 100644 --- a/tools/testing/selftests/arm64/morello/bootstrap.c +++ b/tools/testing/selftests/arm64/morello/bootstrap.c @@ -43,6 +43,15 @@ struct morello_auxv { uintcap_t a_val; }; +struct stack_items_t {

Naming: structs are in their own namespace so we don't use an _t suffix (this is only used for typedefs). Also I'd suggest "initial_data" as name as those things don't have to be on the stack.

• int argc;
• char **argv;
• char **envp;
• struct morello_auxv *auxv;

+};

+static struct stack_items_t stack_items;

Maybe "initial_data_from_regs" to make it very clear what the difference is with the data parsed from the stack in test_stack?

• int clear_child_tid;

void verify_string(char *str) @@ -70,6 +79,9 @@ int verify_auxval(struct morello_auxv *auxv) switch (auxv->a_type) { case AT_NULL: return 0;

• case AT_CHERI_EXEC_RW_CAP:
• case AT_CHERI_INTERP_RW_CAP:
• case AT_CHERI_INTERP_RX_CAP: case AT_BASE: /* Fall through if not null, abi allows it */ if ((void *)auxv->a_val == NULL)

@@ -79,6 +91,12 @@ int verify_auxval(struct morello_auxv *auxv) case AT_PLATFORM: case AT_RANDOM: case AT_PHDR:

• case AT_CHERI_EXEC_RX_CAP:
• case AT_CHERI_STACK_CAP:
• case AT_CHERI_SEAL_CAP:
• case AT_CHERI_CID_CAP:
• case AT_ARGV:
• case AT_ENVP: /* valid and unsealed */ ASSERT_TRUE(cheri_tag_get(auxv->a_val)) TH_LOG("auxv (%ld) value is invalid", auxv->a_type);

@@ -131,48 +149,57 @@ TEST(test_c64) TEST(test_stack) {

• int argc_reg = stack_items.argc;
• char **argv_reg = stack_items.argv;
• char **envp_reg = stack_items.envp;
• struct morello_auxv *auxv_reg = stack_items.auxv;

Considering you use each of these only once, I don't see the point in copying them in local variables. Just access each member of stack_items directly where you need it.

Kevin

• /* copy the pointer so we can modify it */ char **stack = stack_from_kernel;

• int argc;

• int argc_stack; /* start with an argc check */ VERIFY_CAP(stack, sizeof(void *), STACK_PERMS, "argc");

/* dereference and go past argc */

• argc = *(int *)stack;

• argc_stack = *(int *)stack;
• ASSERT_EQ(argc_stack, argc_reg); stack += 1;

/* argv + null */

• VERIFY_CAP(stack, sizeof(void *) * (argc + 1), STACK_PERMS, "argv");

• VERIFY_CAP(stack, sizeof(void *) * (argc_stack + 1), STACK_PERMS, "argv_stack");
• ASSERT_CAP_EQ(argv_reg, stack);

/* we are clear to dereference all argv */

• for (int i = 0; i < argc; i++) {

• for (int i = 0; i < argc_stack; i++) { char *arg = *(stack + i); verify_string(arg); }

/* go past argv */

• stack += argc;
• ASSERT_NULL(*stack) TH_LOG("argv was not null terminated");

• stack += argc_stack;
• ASSERT_NULL(*stack) TH_LOG("argv was not null terminated on stack"); /* go past the null terminator */ stack += 1;

/* progressively check bounds for envp and dereference */

• ASSERT_CAP_EQ(envp_reg, stack); while (1) {

• char *envp = *stack;
  

• char *envp_stack = *stack;
  

VERIFY_CAP(stack, sizeof(void *), STACK_PERMS, "envp"); stack += 1;

• if (envp == NULL)
  

• if (envp_stack == NULL)
  break;
  

• verify_string(envp);
  

• verify_string(envp_stack);
  

  }

/* finally, go through auxv */

• ASSERT_CAP_EQ(auxv_reg, stack); while (1) {

• struct morello_auxv *auxv = ((struct morello_auxv *) stack);
  

• struct morello_auxv *auxv_stack = ((struct morello_auxv *) stack);
  

• VERIFY_CAP(auxv, sizeof(struct morello_auxv), STACK_PERMS, "auxv");
  

• if (verify_auxval(auxv) == 0)
  

• VERIFY_CAP(auxv_stack, sizeof(struct morello_auxv), STACK_PERMS, "auxv");
  

• if (verify_auxval(auxv_stack) == 0)
  break;
  

  stack += 2; }

@@ -192,8 +219,13 @@ TEST(test_set_tid_address_initial)

• exit test. We assume exit() works and we progressively build a known working
• test environment.

*/ -int main(void) +int main(int argc, char **argv, char **envp, struct morello_auxv *auxv) {

• stack_items.argc = argc;
• stack_items.argv = argv;
• stack_items.envp = envp;
• stack_items.auxv = auxv;
• test_write(); /* from now on write and exit work, so go wild */ test_c64();

diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 9eccac44d623..66dfe2bab676 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -66,6 +66,12 @@ VARIABLE(__cur_test, 16) .text FUNCTION_START(_start)

• /* save x0, c1-c3 which has argc/argv/envp/auxv */
• mov x20, x0
• mov c21, c1
• mov c22, c2
• mov c23, c3
• bl __morello_init_globals

/* save the initial kernel stack to a global variable */ @@ -84,7 +90,11 @@ FUNCTION_START(_start) mov csp, c0 #endif

• /* call int main(void) */

• /* call int main(int argc, char **argv, char **envp, char **auxv) */
• mov x0, x20
• mov c1, c21
• mov c2, c22
• mov c3, c23 bl main

/* exit. Argument is in x0 already */