Skip to contentSkip to conversation

arm64: don't set READ_IMPLIES_EXEC for EM_AARCH64

PATCH In review Kyle McMartin 3 participants 3 messages May 14, 2014 – May 15, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm64: don't set READ_IMPLIES_EXEC for EM_AARCH64

current->personality & READ_IMPLIES_EXEC is currently being set for AArch64 binaries, resulting in an executable stack, when no explicit PT_GNU_STACK header is present.

[kmcmarti at sedition ~]$ uname -p
aarch64
[kmcmarti at sedition ~]$ cat /proc/$$/personality 00400000

The reason for this is, without an explicit PT_GNU_STACK entry in the binary, stk is still set to EXSTACK_DEFAULT (which should be non-executable on AArch64.) As a result, elf_read_implies_exec is true, and we set READ_IMPLIES_EXEC in binfmt_elf.c:load_elf_binary.

Fix this to return 0 in the native case, and parrot the logic from arch/arm/kernel/elf.c otherwise. With this patch, binaries correctly don't have READ_IMPLIES_EXEC set, and we can let PT_GNU_STACK change things if it's explicitly requested.

arch/arm64/include/asm/elf.h +2 -1
@@ -114,7 +114,8 @@ typedef struct user_fpsimd_state elf_fpregset_t;
  */
 #define elf_check_arch(x)		((x)->e_machine == EM_AARCH64)
 
-#define elf_read_implies_exec(ex,stk)	(stk != EXSTACK_DISABLE_X)
+#define elf_read_implies_exec(ex,stk)	(test_thread_flag(TIF_32BIT) \
+					 ? (stk == EXSTACK_ENABLE_X) : 0)
 
 #define CORE_DUMP_USE_REGSET
 #define ELF_EXEC_PAGESIZE	PAGE_SIZE
#1 lore raw
Quoted 14 lines · Kyle McMartin

current->personality & READ_IMPLIES_EXEC is currently being set for AArch64 binaries, resulting in an executable stack, when no explicit PT_GNU_STACK header is present.

[kmcmarti at sedition ~]$ uname -p
aarch64
[kmcmarti at sedition ~]$ cat /proc/$$/personality 00400000

The reason for this is, without an explicit PT_GNU_STACK entry in the binary, stk is still set to EXSTACK_DEFAULT (which should be non-executable on AArch64.) As a result, elf_read_implies_exec is true, and we set READ_IMPLIES_EXEC in binfmt_elf.c:load_elf_binary.

Quoted 4 lines

Fix this to return 0 in the native case, and parrot the logic from arch/arm/kernel/elf.c otherwise. With this patch, binaries correctly don't have READ_IMPLIES_EXEC set, and we can let PT_GNU_STACK change things if it's explicitly requested.

Unfortunately, my understanding is that GCC currently requires this for nested functions, so this is an effective ABI breakage. On the plus side, the GCC guys are planning to fix that, so we should see PT_GNU_STACK getting used more frequently in the future.

Will

#2 lore raw
Quoted 5 lines · Will Deacon

Unfortunately, my understanding is that GCC currently requires this for nested functions, so this is an effective ABI breakage. On the plus side, the GCC guys are planning to fix that, so we should see PT_GNU_STACK getting used more frequently in the future.

nested functions are a GNU extension, and aside from grub, I don't think anyone really uses them. (At least, I certainly hope not.) Also, ld.so already has a different idea from the kernel:
master at glibc:.% git grep DEFAULT_STACK_PERMS -- sysdeps/aarch64 (kyle at dreadnought:~/src/glibc) sysdeps/aarch64/stackinfo.h:#define DEFAULT_STACK_PERMS (PF_R|PF_W)

ppc64 also does similarly, where they use the EXSTACK_DEFAULT in the compat case, but their 64-bit ABI doesn't.

108 # define elf_read_implies_exec(ex, exec_stk) (is_32bit_task() ? \
109 (exec_stk == EXSTACK_DEFAULT) : 0)

I've submitted a gcc patch to emit .note.GNU-stack on AArch64 (which I suspect was just an oversight initially.) Which will mean that anything that puts code on the stack will be annotated to require executable stacks.

And, frankly, I think it's kind of strange to have any of the personality bits set by default.

regards, Kyle