Hello all,
Apologies for taking so long to send a v2. It needed a few big reworks for which finding time was difficult. But they are now done and all the comments that were raised on the v1 are hopefully addressed, as are the few issues I raised myself.
Hopefully now that I have everything set-up again and changes should be smaller, I should be able to address comments much quicker !
This patch properly restricts the bounds of argv and envp strings in purecap. It handles the padding and alignment changes required for setting exact bounds. The strings are still passed to userspace on the stack. Changing this would be covered by some future work.
# Remarks
I am a bit unsure of a few things. Mainly, how the capabilities passed to `put_str_array()` are copied and need updating outside, and the way I force the alignment of the stack in `setup_arg_pages()`.
# Testing
For validation purposes, LTP was run in purecap with the full syscalls suite and the morello skip lists, with no regression observed. For quickly testing the patch, one can build a program trying to access an argv outside of its bounds. Here is a sample program I used for testing.
#include <stdio.h> #include <stdlib.h> #include <string.h> #include <inttypes.h>
int main(int argc, char** argv) {
printf("argc : %d\n", argc); for (int i = 0; i < argc; i++) { size_t arglen = strlen(argv[i]); printf("arg #%d (len %ld):\n", i, arglen); printf("\t%s\n", argv[i]); }
for (int i = 0; i < argc; i++) { size_t arglen = strlen(argv[i]); printf("arg #%d @ %ld:\n", i, arglen+5); printf("\t%c\n", argv[i][arglen+5]); }
return 0; }
Without the patch or in COMPAT : all the args are printed, and argc number of out of bounds characters are printed. With the patch, in purecap : all the args are printed, but the program SEGFAULTS before the first out of bounds character is printed.
You can generate a large enough argument that would need padding with this :
bigarg=""; count=0; while [ $(echo $bigarg | wc -c) -le 20000 ]; do bigarg=${bigarg}"==${count}==This_should_be_a_really_big_arg_string" count=$(($count + 1)) done
Thanks in advance for your comments, Best regards Téo
# Changes from v1[0]
- Rebased on top of the last release - Update to make use of the properly derived stack capability in binfmt_elf - Move the copying of argv and envp strings in binfmt_elf to a helper function - Check for padding after an arg only - Greatly simplify the change in exec by completely skipping the padding, rather than looping through it and allocating pages in exec - Add more details to the comments explaining the padding process - Rename most variables. I'm not great with names so hopefully they are OK, otherwise feel free to suggest new ones ! - Proper COMPAT handling, but with a slight loss compared to a regular kernel - Detail trade-off in comments and in commit message - Force a greater stack alignment in exec to mitigate issues during relocation - Simplify accesses and use `get_user()` rather than `copy_from_user()` - Use wrappers provided by <cheriintrin.h> rather than builtins - Get rid of the elf_stack_put_user_cap macro
Gitlab patch for review : https://git.morello-project.org/Teo-CD/linux/-/commit/089b18d666efd567530a7b...
[0]: https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Teo Couprie Diaz (1): fs: Handle exact bounds for argv and envp
fs/binfmt_elf.c | 111 ++++++++++++++++++++++++++++++++++++++++-------- fs/exec.c | 50 ++++++++++++++++++++-- 2 files changed, 139 insertions(+), 22 deletions(-)