Hi,
This short series refactors the way pointers to the stack are manipulated in binfmt_elf. The changes are generic and arguably improve binfmt_elf, but the main objective is to eliminate unnecessary creation of capabilities in PCuABI (through calls to uaddr_to_user_ptr_safe()). This is done by using an actual user pointer to keep track of the current position on the stack, and writing all data through that pointer, instead of using an addresss and creating a new user pointer for every access. This is what patch 1 does. Patch 2 simplifies the elf_stack_put_user* macros we previously introduced, as we do not need them to do something special in PCuABI any more.
This series should help with further work on restricting initial capabilities [1]. It does not have any user-visible effect itself however. The new "root stack capability" is still unrestricted, but the fact that all capabilities to the stack are derived from it means that any later narrowing of its bounds or permissions will automatically propagate.
Note that these changes are mostly orthogonal to Téo's series [2] that partially addresses [1]; it just means that using uaddr_to_user_ptr_safe() is no longer necessary to derive the argv / envp capabilities.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/binfmt_...
Thanks, Kevin
[1] https://git.morello-project.org/morello/kernel/linux/-/issues/19 [2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Kevin Brodsky (2): fs/binfmt_elf: Improve SP manipulation in PCuABI fs/binfmt_elf: Simplify elf_stack_put_user*
fs/binfmt_elf.c | 85 +++++++++++++++++++++++------------------- fs/compat_binfmt_elf.c | 9 +---- include/linux/elf.h | 12 +----- 3 files changed, 48 insertions(+), 58 deletions(-)
In PCuABI, we should derive capabilities from the user root capability as rarely as possible. The ELF loader currently does this many times in create_elf_tables(), as it represents positions on the stack as simple addresses in many cases.
This patch improves the situation by deriving all pointers to the stack from a single stack pointer variable (sp). As a result uaddr_to_user_ptr_safe() is now only used once to initialise sp.
The type of sp is changed to void __user * as it does not actually point to aligned data at the beginning of the function (STACK_ALLOC() allocates a string on the stack without any particular alignment). Similarly, the type of the u_* variables is changed to char __user * as they point to these (unaligned) strings, not arrays of elf_addr_t. Once sp has been aligned using STACK_ROUND(), a new pointer stack_item, of the appropriate type, is used to write the pointer-sized "stack items".
This patch should not induce any user-visible change.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- fs/binfmt_elf.c | 85 +++++++++++++++++++++++------------------- fs/compat_binfmt_elf.c | 2 +- include/linux/elf.h | 5 +-- 3 files changed, 49 insertions(+), 43 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 8719aeef5f9c..fbe83ed56531 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -153,23 +153,20 @@ static int padzero(unsigned long elf_bss) }
/* Let's use some macros to make this stack manipulation a little clearer */ -/* - * TODO [PCuABI]: The sp pointer can ideally be a capability pointer so that - * the macro defined here need not create capability. - */ #ifdef CONFIG_STACK_GROWSUP -#define STACK_ADD(sp, items) ((elf_stack_item_t __user *)uaddr_to_user_ptr_safe(sp) + (items)) +#define STACK_ADD(sp, items) ((sp) + (items) * sizeof(elf_stack_item_t)) #define STACK_ROUND(sp, items) \ - ((15 + user_ptr_addr((sp) + (items))) &~ 15UL) + USER_PTR_ALIGN((sp) + (items) * sizeof(elf_stack_item_t), 16) #define STACK_ALLOC(sp, len) ({ \ - elf_addr_t __user *old_sp = uaddr_to_user_ptr_safe(sp); sp += len; \ + void __user *old_sp = sp; sp += len; \ old_sp; }) #else -#define STACK_ADD(sp, items) ((elf_stack_item_t __user *)uaddr_to_user_ptr_safe(sp) - (items)) +#define STACK_ADD(sp, items) ((sp) - (items) * sizeof(elf_stack_item_t)) #define STACK_ROUND(sp, items) \ - (user_ptr_addr((sp) - (items)) &~ 15UL) -#define STACK_ALLOC(sp, len) ({ sp -= len ; (elf_addr_t __user *)uaddr_to_user_ptr_safe(sp); }) + USER_PTR_ALIGN_DOWN((sp) - (items) * sizeof(elf_stack_item_t), 16) +#define STACK_ALLOC(sp, len) (sp -= len) #endif +static_assert(sizeof(elf_stack_item_t) <= 16);
#ifndef ELF_BASE_PLATFORM /* @@ -198,10 +195,11 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, unsigned long p = bprm->p; int argc = bprm->argc; int envc = bprm->envc; - elf_stack_item_t __user *sp; - elf_addr_t __user *u_platform; - elf_addr_t __user *u_base_platform; - elf_addr_t __user *u_rand_bytes; + void __user *sp; + elf_stack_item_t __user *stack_item; + char __user *u_platform; + char __user *u_base_platform; + char __user *u_rand_bytes; const char *k_platform = ELF_PLATFORM; const char *k_base_platform = ELF_BASE_PLATFORM; unsigned char k_rand_bytes[16]; @@ -211,6 +209,7 @@ 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; + char __user *ustr; #if defined(CONFIG_CHERI_PURECAP_UABI) && (ELF_COMPAT == 0) elf_stack_item_t *mm_at_argv, *mm_at_envp; #endif @@ -223,6 +222,12 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
p = arch_align_stack(p);
+ /* + * TODO [PCuABI] - derive an appropriate capability (bounds matching + * the stack reservation, RW permissions) + */ + sp = uaddr_to_user_ptr_safe(p); + /* * If this architecture has a platform capability string, copy it * to userspace. In some cases (Sparc), this info is impossible @@ -233,7 +238,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, if (k_platform) { size_t len = strlen(k_platform) + 1;
- u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len); + u_platform = STACK_ALLOC(sp, len); if (copy_to_user(u_platform, k_platform, len)) return -EFAULT; } @@ -246,7 +251,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, if (k_base_platform) { size_t len = strlen(k_base_platform) + 1;
- u_base_platform = (elf_addr_t __user *)STACK_ALLOC(p, len); + u_base_platform = STACK_ALLOC(sp, len); if (copy_to_user(u_base_platform, k_base_platform, len)) return -EFAULT; } @@ -255,8 +260,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, * Generate 16 random bytes for userspace PRNG seeding. */ get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes)); - u_rand_bytes = (elf_addr_t __user *) - STACK_ALLOC(p, sizeof(k_rand_bytes)); + u_rand_bytes = STACK_ALLOC(sp, sizeof(k_rand_bytes)); if (copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes))) return -EFAULT;
@@ -356,17 +360,16 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, elf_info += 2;
ei_index = elf_info - (elf_stack_item_t *)mm->saved_auxv; - sp = STACK_ADD(p, ei_index); + sp = STACK_ADD(sp, ei_index);
items = (argc + 1) + (envc + 1) + 1; - bprm->p = STACK_ROUND(sp, items); + sp = STACK_ROUND(sp, items); + bprm->p = user_ptr_addr(sp);
/* Point sp at the lowest address on the stack */ #ifdef CONFIG_STACK_GROWSUP - sp = (elf_stack_item_t __user *)uaddr_to_user_ptr_safe(bprm->p) - items - ei_index; + sp -= (items + ei_index) * sizeof(elf_stack_item_t); bprm->exec = user_ptr_addr(sp); /* XXX: PARISC HACK */ -#else - sp = (elf_stack_item_t __user *)uaddr_to_user_ptr_safe(bprm->p); #endif
@@ -382,47 +385,51 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, return -EFAULT;
/* Now, let's put argc (and argv, envp if appropriate) on the stack */ - if (elf_stack_put_user(argc, sp++)) + stack_item = sp; + + if (elf_stack_put_user(argc, stack_item++)) 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; + *mm_at_argv = (elf_stack_item_t)stack_item; #endif - p = mm->arg_end = mm->arg_start; + /* In PCuABI, this derives a capability from SP pointing to arg_start */ + ustr = sp + (mm->arg_start - user_ptr_addr(sp)); while (argc-- > 0) { size_t len; - if (elf_stack_put_user_ptr(p, sp++)) + if (elf_stack_put_user_ptr(ustr, stack_item++)) return -EFAULT; - len = strnlen_user(uaddr_to_user_ptr_safe(p), MAX_ARG_STRLEN); + len = strnlen_user(ustr, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) return -EINVAL; - p += len; + ustr += len; } - if (elf_stack_put_user(0, sp++)) + if (elf_stack_put_user(0, stack_item++)) return -EFAULT; - mm->arg_end = p; + mm->arg_end = user_ptr_addr(ustr);
/* 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; + *mm_at_envp = (elf_stack_item_t)stack_item; #endif - mm->env_end = mm->env_start = p; + mm->env_start = user_ptr_addr(ustr); while (envc-- > 0) { size_t len; - if (elf_stack_put_user_ptr(p, sp++)) + if (elf_stack_put_user_ptr(ustr, stack_item++)) return -EFAULT; - len = strnlen_user(uaddr_to_user_ptr_safe(p), MAX_ARG_STRLEN); + len = strnlen_user(ustr, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) return -EINVAL; - p += len; + ustr += len; } - if (elf_stack_put_user(0, sp++)) + if (elf_stack_put_user(0, stack_item++)) return -EFAULT; - mm->env_end = p; + mm->env_end = user_ptr_addr(ustr);
/* Put the elf_info on the stack in the right place. */ - if (elf_copy_to_user_stack(sp, mm->saved_auxv, ei_index * sizeof(elf_stack_item_t))) + if (elf_copy_to_user_stack(stack_item, mm->saved_auxv, + ei_index * sizeof(elf_stack_item_t))) return -EFAULT; return 0; } diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c index b6b453b35946..062ab93e9503 100644 --- a/fs/compat_binfmt_elf.c +++ b/fs/compat_binfmt_elf.c @@ -44,7 +44,7 @@ #define elf_copy_to_user_stack(to, from, len) copy_to_user(to, from, len)
#undef elf_stack_put_user_ptr -#define elf_stack_put_user_ptr(val, ptr) put_user((elf_addr_t)val, ptr) +#define elf_stack_put_user_ptr(val, ptr) put_user(user_ptr_addr(val), ptr)
#undef elf_stack_put_user #define elf_stack_put_user(val, ptr) put_user(val, ptr) diff --git a/include/linux/elf.h b/include/linux/elf.h index 039ad1867045..5d936561a5aa 100644 --- a/include/linux/elf.h +++ b/include/linux/elf.h @@ -69,13 +69,12 @@ extern Elf64_Dyn _DYNAMIC []; #define elf_copy_to_user_stack(to, from, len) copy_to_user_with_ptr(to, from, len) #ifdef CONFIG_CHERI_PURECAP_UABI #define elf_stack_put_user_ptr(val, ptr) \ - put_user_ptr(elf_uaddr_to_user_ptr(val), \ - (void * __capability * __capability)ptr) + put_user_ptr((val), (void * __capability * __capability)ptr) #define elf_stack_put_user(val, ptr) \ put_user_ptr(as_user_ptr(val), \ (void * __capability * __capability)ptr) #else -#define elf_stack_put_user_ptr(val, ptr) put_user((elf_addr_t)val, ptr) +#define elf_stack_put_user_ptr(val, ptr) put_user(user_ptr_addr(val), ptr) #define elf_stack_put_user(val, ptr) put_user(val, ptr) #endif
elf_stack_put_user{,_ptr} both write pointer-sized items to the new user stack. The need for two distinct functions mainly stemmed from the additional "magic" elf_stack_put_user_ptr() had to perform in PCuABI, that is create a valid capability out of the specified address. This also meant that the functions are implemented three times (PCuABI, !PCuABI, compat).
Since the previous patch, this requirement no longer exists, as an actual user pointer is passed to elf_stack_put_user_ptr(). This allows us to make things a lot simpler: only one function, elf_stack_put_user(), with a unique native implementation. A couple of observations make this possible:
* The items being stored are always pointer-sized, as mentioned previously, and it is therefore appropriate to use put_user_ptr() regardless of the ABI.
* Any integer or pointer can be safely cast to user_uintptr_t, and put_user_ptr() is able to manipulate a user_uintptr_t instead of a void __user *, avoiding the awkward cast in PCuABI.
The compat implementation is still needed as items are compat-pointer-sized in compat, in other words smaller than a native user pointer; thus, put_user_ptr() is not appropriate in that case.
While at it, elf_copy_to_user_stack() is now also defined unconditionally in compat, regardless of PCuABI. This makes no difference as copy_to_user_with_ptr() is equivalent to copy_to_user() in !PCuABI.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- fs/binfmt_elf.c | 4 ++-- fs/compat_binfmt_elf.c | 9 +-------- include/linux/elf.h | 11 +---------- 3 files changed, 4 insertions(+), 20 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index fbe83ed56531..6455313d7f36 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -398,7 +398,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, ustr = sp + (mm->arg_start - user_ptr_addr(sp)); while (argc-- > 0) { size_t len; - if (elf_stack_put_user_ptr(ustr, stack_item++)) + if (elf_stack_put_user(ustr, stack_item++)) return -EFAULT; len = strnlen_user(ustr, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) @@ -416,7 +416,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, mm->env_start = user_ptr_addr(ustr); while (envc-- > 0) { size_t len; - if (elf_stack_put_user_ptr(ustr, stack_item++)) + if (elf_stack_put_user(ustr, stack_item++)) return -EFAULT; len = strnlen_user(ustr, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c index 062ab93e9503..5495cede2975 100644 --- a/fs/compat_binfmt_elf.c +++ b/fs/compat_binfmt_elf.c @@ -38,18 +38,11 @@ #undef elf_uaddr_to_user_ptr #define elf_uaddr_to_user_ptr(addr) addr
-#ifdef CONFIG_CHERI_PURECAP_UABI - #undef elf_copy_to_user_stack #define elf_copy_to_user_stack(to, from, len) copy_to_user(to, from, len)
-#undef elf_stack_put_user_ptr -#define elf_stack_put_user_ptr(val, ptr) put_user(user_ptr_addr(val), ptr) - #undef elf_stack_put_user -#define elf_stack_put_user(val, ptr) put_user(val, ptr) - -#endif /* CONFIG_CHERI_PURECAP_UABI */ +#define elf_stack_put_user(val, ptr) put_user((elf_addr_t)(user_uintptr_t)(val), (ptr))
#ifdef CONFIG_COMPAT32 /* diff --git a/include/linux/elf.h b/include/linux/elf.h index 5d936561a5aa..abfa9b96259b 100644 --- a/include/linux/elf.h +++ b/include/linux/elf.h @@ -67,16 +67,7 @@ extern Elf64_Dyn _DYNAMIC []; #define elf_stack_item_t user_uintptr_t #define elf_uaddr_to_user_ptr(addr) uaddr_to_user_ptr_safe(addr) #define elf_copy_to_user_stack(to, from, len) copy_to_user_with_ptr(to, from, len) -#ifdef CONFIG_CHERI_PURECAP_UABI -#define elf_stack_put_user_ptr(val, ptr) \ - put_user_ptr((val), (void * __capability * __capability)ptr) -#define elf_stack_put_user(val, ptr) \ - put_user_ptr(as_user_ptr(val), \ - (void * __capability * __capability)ptr) -#else -#define elf_stack_put_user_ptr(val, ptr) put_user(user_ptr_addr(val), ptr) -#define elf_stack_put_user(val, ptr) put_user(val, ptr) -#endif +#define elf_stack_put_user(val, ptr) put_user_ptr((user_uintptr_t)(val), (ptr))
/* Optional callbacks to write extra ELF notes. */ struct file;
On 17-08-2023 10:42, Kevin Brodsky wrote:
Hi,
This short series refactors the way pointers to the stack are manipulated in binfmt_elf. The changes are generic and arguably improve binfmt_elf, but the main objective is to eliminate unnecessary creation of capabilities in PCuABI (through calls to uaddr_to_user_ptr_safe()). This is done by using an actual user pointer to keep track of the current position on the stack, and writing all data through that pointer, instead of using an addresss and creating a new user pointer for every access. This is what patch 1 does. Patch 2 simplifies the elf_stack_put_user* macros we previously introduced, as we do not need them to do something special in PCuABI any more.
This series should help with further work on restricting initial capabilities [1]. It does not have any user-visible effect itself however. The new "root stack capability" is still unrestricted, but the fact that all capabilities to the stack are derived from it means that any later narrowing of its bounds or permissions will automatically propagate.
Note that these changes are mostly orthogonal to Téo's series [2] that partially addresses [1]; it just means that using uaddr_to_user_ptr_safe() is no longer necessary to derive the argv / envp capabilities.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/binfmt_...
Thanks, Kevin
Very nice cleanup! Looks good!
Tudor
[1] https://git.morello-project.org/morello/kernel/linux/-/issues/19 [2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Kevin Brodsky (2): fs/binfmt_elf: Improve SP manipulation in PCuABI fs/binfmt_elf: Simplify elf_stack_put_user*
fs/binfmt_elf.c | 85 +++++++++++++++++++++++------------------- fs/compat_binfmt_elf.c | 9 +---- include/linux/elf.h | 12 +----- 3 files changed, 48 insertions(+), 58 deletions(-)
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Tudor Cretu