On 12/01/2023 09:36, Amit Daniel Kachhap wrote:
Gcc emits only dynamic relocations at the moment and not cap relocations
I would remove "at the moment": emitting dynamic relocations is the new approach, there is no plan for GCC to emit caprelocs and Clang should be moving to dynamic relocations eventually too.
like Clang toolchain. Hence the existing kselftests compiled in static and standalone mode needs to process the dynamic relocations and fill the entries to relevant capability details.
The code changes are similar to the start up code of the recently released Arm GNU Toolchain for Morello.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
.../arm64/morello/freestanding_init_globals.c | 148 +++++++++++++++++- .../arm64/morello/freestanding_start.S | 5 +- 2 files changed, 146 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c index 6139de724f24..243e9d3323ff 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c +++ b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c @@ -5,8 +5,47 @@ #include <stdbool.h> #include <stddef.h> #include <stdint.h> +#include <linux/auxvec.h> +#include <linux/elf.h>
#include "freestanding.h" +#define CHERI_PERM_MASK_BASE (-1UL ^ ( \
- CHERI_PERM_LOAD | \
- CHERI_PERM_STORE | \
- CHERI_PERM_EXECUTE | \
- CHERI_PERM_LOAD_CAP | \
- ARM_CAP_PERMISSION_MUTABLE_LOAD | \
- CHERI_PERM_STORE_CAP | \
- CHERI_PERM_STORE_LOCAL_CAP | \
- ARM_CAP_PERMISSION_EXECUTIVE | \
- CHERI_PERM_SYSTEM_REGS | \
- CHERI_PERM_SEAL | \
- CHERI_PERM_UNSEAL | \
- ARM_CAP_PERMISSION_COMPARTMENT_ID))
I'm not sure I understand the point of this, can't we just use Global as base?
+#define CHERI_PERM_MASK_R ( \
- CHERI_PERM_MASK_BASE | \
- CHERI_PERM_LOAD | \
- CHERI_PERM_LOAD_CAP | \
- ARM_CAP_PERMISSION_MUTABLE_LOAD)
+#define CHERI_PERM_MASK_RW ( \
- CHERI_PERM_MASK_R | \
- CHERI_PERM_STORE | \
- CHERI_PERM_STORE_CAP | \
- CHERI_PERM_STORE_LOCAL_CAP)
+#define CHERI_PERM_MASK_RX ( \
- CHERI_PERM_MASK_R | \
- CHERI_PERM_EXECUTE | \
- ARM_CAP_PERMISSION_EXECUTIVE | \
- CHERI_PERM_SYSTEM_REGS)
+#ifndef R_MORELLO_RELATIVE +#define R_MORELLO_RELATIVE 59395 +#endif
struct cap_reloc { size_t capability_location; size_t base; @@ -23,7 +62,7 @@ struct cap_reloc {
- because capability relocations must have already been processed in order to
- refer to such symbols.
*/ -void __morello_init_globals(void) +void __morello_init_cap_relocs(void) { const struct cap_reloc *start_cap_relocs, *end_cap_relocs, *reloc; uintcap_t root_cap; @@ -33,11 +72,20 @@ void __morello_init_globals(void) * not be indirected through the GOT, as this would create a capability * relocation. We need assembly to refer to those directly. */
- asm("adrp %0, __start___cap_relocs\n\t"
- asm(".weak __start___cap_relocs\n\t"
".hidden __start___cap_relocs\n\t""adrp %0, __start___cap_relocs\n\t" "add %0, %0, #:lo12:__start___cap_relocs\n\t"
"adrp %1, __stop___cap_relocs\n\t""add %1, %1, #:lo12:__stop___cap_relocs": "=C"(start_cap_relocs), "=C"(end_cap_relocs));
: "=C"(start_cap_relocs));- asm(".weak __stop___cap_relocs\n\t"
".hidden __stop___cap_relocs\n\t""adrp %0, __stop___cap_relocs\n\t""add %0, %0, #:lo12:__stop___cap_relocs": "=C"(end_cap_relocs));- if (start_cap_relocs >= end_cap_relocs)
return;
Isn't that implicitly tested in the loop below already? This does skip the next line, but since it should be just one instruction I'm not sure it's worth bothering.
root_cap = (uintcap_t)cheri_ddc_get(); @@ -63,3 +111,93 @@ void __morello_init_globals(void) *target = cap; } }
+static void
Nit: it's uncommon to use this style in the kernel, better have the return type on the same line and split the argument list if necessary. Same below.
+get_caps(uintptr_t *cap_rx, uintptr_t *cap_rw, const uintptr_t *auxv) +{
- for (;;) {
switch ((unsigned long)auxv[0]) {case AT_NULL:return;case AT_CHERI_EXEC_RX_CAP:*cap_rx = auxv[1];*cap_rx = cheri_perms_and(*cap_rx, CHERI_PERM_MASK_RX);break;case AT_CHERI_EXEC_RW_CAP:*cap_rw = auxv[1];*cap_rw = cheri_perms_and(*cap_rw, CHERI_PERM_MASK_RW);break;}auxv += 2;- }
+}
+static inline uintptr_t +morello_relative(uint64_t base, uintptr_t cap_rx, uintptr_t cap_rw,
Elf64_Rela *reloc, void *reloc_addr)+{
- uint64_t *__attribute__((may_alias)) u64_reloc_addr = reloc_addr;
Nit: space between * and __attribute__.
More importantly though, isn't the attribute mispositioned? AIUI this is a type attribute, meaning that the type may alias another type when accessed through a pointer. The aliasing we want to allow here is between pointers to uint64_t and uintptr_t, not between pointers to uint64_t * and uintptr_t * (see also [1]).
[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html
- /*
* Fragment identified by r_offset has the following information:* | 64-bit: address | 56-bits: length | 8-bits: permissions |*/- unsigned long loc = u64_reloc_addr[0];
- unsigned long len = u64_reloc_addr[1] & ((1UL << 56) - 1);
- unsigned long perm = u64_reloc_addr[1] >> 56;
- uintptr_t value;
- /*
* Permissions field is encoded as:* 4 = executable, 2 = read/write, 1 = read-only.*/- if (perm == 2)
value = cheri_address_set(cap_rw, base + loc);- else
value = cheri_address_set(cap_rx, base + loc);- value = cheri_bounds_set_exact(value, len);
- value = value + reloc->r_addend;
- if (perm == 1)
value = cheri_perms_and(value, CHERI_PERM_MASK_R);- /* Seal executable capabilities with MORELLO_RB. */
- if (perm == 4)
value = cheri_sentry_create(value);- return value;
+}
+void __morello_init_dynamic_relocs(int __attribute__((unused)) argc,
char __attribute__((unused)) **argv,char __attribute__((unused)) **envp,
Nit: putting the attribute here seems to work, but that's a rather strange position. After the argument name is more common.
void *auxv)+{
- Elf64_Rela *rela_dyn_start, *rela_dyn_end, *reloc;
- uintptr_t cap_rx = 0;
- uintptr_t cap_rw = 0;
- asm(".weak __rela_dyn_start\n\t"
".hidden __rela_dyn_start\n\t""adrp %0, __rela_dyn_start\n\t""add %0, %0, #:lo12:__rela_dyn_start\n\t": "=C"(rela_dyn_start));- asm(".weak __rela_dyn_end\n\t"
".hidden __rela_dyn_end\n\t""adrp %0, __rela_dyn_end\n\t""add %0, %0, #:lo12:__rela_dyn_end": "=C"(rela_dyn_end));- if (rela_dyn_start >= rela_dyn_end)
return;- get_caps(&cap_rx, &cap_rw, auxv);
- for (reloc = rela_dyn_start; reloc < rela_dyn_end; ++reloc) {
uintptr_t *reloc_addr, value;if (reloc->r_info != R_MORELLO_RELATIVE)continue;reloc_addr = (uintptr_t *)cheri_address_set(cap_rw, reloc->r_offset);value = morello_relative(0, cap_rx, cap_rw, reloc, reloc_addr);*reloc_addr = value;- }
- /* Compiler barrier after relocs are processed. */
- asm volatile ("" ::: "memory");
This is called from assembly, so no compiler barrier is needed.
+} diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 66dfe2bab676..f04cd7cad368 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -25,7 +25,7 @@
- besides the .space we specify:
- a space for a capability pointing to it (in the .got section)
- a capability relocation (in __cap_relocs section) to make the capability
- valid (see __morello_init_globals() for more details)
- valid (see __morello_init_cap_relocs() for more details)
Good you spotted this, I would also remove the parentheses in the line above, and here say e.g. __morello__init_{cap,dynamic}_relocs to make it clear there are two possible forms of relocations.
- We use
- adrp c0, :got:name
- ldr c0, [c0, :got_lo12:name]
@@ -72,7 +72,8 @@ FUNCTION_START(_start) mov c22, c2 mov c23, c3
- bl __morello_init_globals
- bl __morello_init_cap_relocs
- bl __morello_init_dynamic_relocs
I think "__morello_process_*_relocs" would make more sense ("init" was about initialising globals, now we say we talk about processing relocations instead). Same observation concerning the commit title for that matter.
Kevin
/* save the initial kernel stack to a global variable */ got_get_ptr stack_from_kernel, c0