i#1973: musl: Scan stack for kernel arguments when used as library #7266
Conversation
Musl doesn't pass any arguments to constructors. To obtain environment variables for initialization of dynamorio, we scan the stack and search for specific patterns in the auxiliary vector passed by kernel, then walk back towards the top and find the environment variables. We choose to match AT_EXECFN, which has been passed unconditionally by the kernel from 2012. Its value should be a valid address, providing an extra check. This makes using dynamorio as a shared library possible on musl thus fixes several testcases. The logic could be enabled as fallback on older Android platforms as well. Issue: DynamoRIO#1973
| # define EFAULT 14 | ||
| # define AT_EXECFN 31 | ||
| static int | ||
| check_address_readable(void *addr) |
There was a problem hiding this comment.
This is what d_r_safe_read is for: please use that instead, as it is faster and is the convention for handling potentially faulting reads.
| * envp | ||
| * NULL (end of envp) | ||
| * auxv | ||
| * search_auxvector() walks towards the higher address and locate one of the |
There was a problem hiding this comment.
grammar nit: s/locate/locates/
| * NULL (end of envp) | ||
| * auxv | ||
| * search_auxvector() walks towards the higher address and locate one of the | ||
| * auxvector entry, then walk backwards and find the beginning of auxvector. */ |
There was a problem hiding this comment.
grammar nit: s/walk/walks/;s/find/finds/
| @@ -788,6 +788,82 @@ static init_fn_t | |||
| #else | |||
| /* If we're a normal shared object, then we override _init. | |||
| */ | |||
|
|
|||
| # if defined(MUSL) | |||
| # define EFAULT 14 | |||
There was a problem hiding this comment.
Isn't this in errno.h (or some header it includes)? Safer to have inside #ifndef EFAULT?
|
|
||
| # if defined(MUSL) | ||
| # define EFAULT 14 | ||
| # define AT_EXECFN 31 |
There was a problem hiding this comment.
This is in elf.h: also safest to have inside ifndef?
| * NULL (end of envp) | ||
| * auxv | ||
| * search_auxvector() walks towards the higher address and locate one of the | ||
| * auxvector entry, then walk backwards and find the beginning of auxvector. */ |
There was a problem hiding this comment.
grammar nit: s/entry/entries/
| * NULL (end of envp) | ||
| * auxv | ||
| * search_auxvector() walks towards the higher address and locate one of the | ||
| * auxvector entry, then walk backwards and find the beginning of auxvector. */ |
There was a problem hiding this comment.
But if the stack pointer is at argc, we know the argv pointer count and we can walk forward to find envp and from there walk forward to auxv (xref privload_setup_auxv() which walks auxv from envp) -- that's cleaner than the code below. Am I missing something?
There was a problem hiding this comment.
But if the stack pointer is at argc,
This is the case when we entering the e_entry of an ELF executable, not our _init() function. I should make this clearer in the comment.
we know the argv pointer count
In our _init entry, the stack pointer has gone far away towards the low address from the state at the ELF entrypoint. As argc is an undeterministic, usually-small value, we cannot distinguish it from other random slots on the stack practically. It's more safe to match a pattern that is always present in the auxvector.
and we can walk forward to find envp and from there walk forward to auxv (xref privload_setup_auxv() which walks auxv from envp) -- that's cleaner than the code below. Am I missing something?
There was a problem hiding this comment.
I see: if you could update the comment as it made me think the stack was right there.
| for (size_t offset = 0; offset < PAGE_SIZE * 64; offset += sizeof(ulong)) { | ||
| ELF_AUXV_TYPE *p = sp + offset; | ||
|
|
||
| if (((uintptr_t)(&p->a_un) & (PAGE_SIZE - 1)) == 0 && |
| @@ -788,6 +788,82 @@ static init_fn_t | |||
| #else | |||
There was a problem hiding this comment.
The logic could be enabled as fallback on older Android platforms later as well.
Note that get_kernel_args() in loader_android.c for older Android finds argc, argv, and envp inside a structure pointed at by TLS which I believe is specific to Android.
| ASSERT_MESSAGE(CHKLVL_ASSERTS, "failed to find auxv", auxv != NULL); | ||
|
|
||
| ulong *p = &auxv[-2]; | ||
| for (; p[-1] && &p[-1] > sp; p--) |
There was a problem hiding this comment.
style: We use explicit conversions to bool: so p[-1] != NULL
|
|
||
| /* Check for AT_EXECFN entry in the auxvector, which contains pathname | ||
| * of the program and should be a readable address. */ | ||
| if (p->a_type == AT_EXECFN && check_address_readable((void *)p->a_un.a_val)) { |
There was a problem hiding this comment.
Is this a strong enough check? An integer 31 adjacent to a valid pointer? It seems like this could happen to appear somewhere else on the stack.
| * NULL (end of envp) | ||
| * auxv | ||
| * search_auxvector() walks towards the higher address and locate one of the | ||
| * auxvector entry, then walk backwards and find the beginning of auxvector. */ |
There was a problem hiding this comment.
I see: if you could update the comment as it made me think the stack was right there.
Musl doesn't pass any arguments to constructors. To obtain environment variables for initialization of dynamorio, we scan the stack and search for specific patterns in the auxiliary vector passed by kernel, then walk back towards the top and find the environment variables.
We choose to match AT_EXECFN, which has been passed unconditionally by the kernel from 2012. Its value should be a valid address, providing an extra check.
This makes using dynamorio as a shared library possible on musl thus fixes several testcases. The logic could be enabled as fallback on older Android platforms later as well.
Issue: #1973