Page MenuHomeFreeBSD

execvPe: obviate the need for potentially large stack allocations
ClosedPublic

Authored by kevans on May 28 2020, 4:58 PM.
Tags
None
Referenced Files
F80136109: D25038.diff
Thu, Mar 28, 10:06 AM
Unknown Object (File)
Thu, Mar 21, 5:21 AM
Unknown Object (File)
Thu, Mar 21, 5:21 AM
Unknown Object (File)
Thu, Mar 21, 5:21 AM
Unknown Object (File)
Thu, Mar 21, 5:21 AM
Unknown Object (File)
Dec 20 2023, 8:16 AM
Unknown Object (File)
Dec 16 2023, 7:18 PM
Unknown Object (File)
Dec 10 2023, 7:50 PM

Details

Summary

Some environments in which execvPe may be called have a limited amount of stack available. Currently, it avoidably allocates a segment on the stack large enough to hold PATH so that it may be mutated and use strsep() for easy parsing. This logic is now rewritten to just operate on the immutable string passed in and do the necessary math to extract individual paths, since it will be copying out those segments to another buffer anyways and piecing them together with the name for a full path.

Additional size is also needed for the stack in posix_spawnp(), because it may need to push all of argv to the stack and rebuild the command with sh in front of it. We'll make sure it's properly aligned for the new thread, but future work should likely make rfork_thread a little easier to use by ensuring proper alignment.

Some trivial cleanup has been done with a couple of error writes, moving strings into char arrays for use with the less fragile sizeof().

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 31354

Event Timeline

kevans created this revision.
kevans created this object with visibility "Custom Policy".
gordon added a subscriber: gordon.

Adding releng. They would deal with in-progress releases, not secteam. We get to deal with them *after* release. :-)

kib added inline comments.
lib/libc/gen/exec.c
212–213

I think this line (and 206) can be improved by moving the string to const array and using sizeof().

This revision is now accepted and ready to land.May 28 2020, 6:21 PM
kevans edited the summary of this revision. (Show Details)

Address the second alloca() that could be a problem if _execve fails with ENOEXEC and we happened to have a lot of argv. This is a little harder to solve with a local write in execvPe without forcing extra work on other consumers, so just make sure we have enough space for our original 4k + whatever could be allocated there in the worst case.

While we're here, push some constants+magic out into a const array for a drive-by improvement.

This revision now requires review to proceed.May 28 2020, 8:12 PM
jilles requested changes to this revision.May 28 2020, 8:50 PM
jilles added inline comments.
lib/libc/gen/exec.c
163–164

There needs to be op = NULL; here so that the path search loop is not entered with uninitialized op if the _execve() fails with ENOENT or a similar error.

238

This is an alloca() as well, for which the stack allocated by do_posix_spawn() may not be sufficient. Perhaps do_posix_spawnp() should reserve enough space for this allocation if it should happen.

This revision now requires changes to proceed.May 28 2020, 8:50 PM
kevans added inline comments.
lib/libc/gen/exec.c
163–164

Good catch, will fix

238

There is a follow-up update that did include this. =)

lib/libc/gen/exec.c
183

On second look this logic looks like it could be a lot simpler; as far as I can tell the two blocks for leading/trailing/double colon can be merged, and so can the ones for final and non-final components: something like,

if (np == op) {
    /* empty component: */
    p = ".";
    lp = 1;
} else {
    /* non-empty component: */
    p = op;
    lp = np - op;
}

or am I missing something?

lib/libc/gen/exec.c
183

Whoops- looks like I forgot to re-evaluate and simplify after strchr -> strchrnul

kevans marked 3 inline comments as done.
  • Restore the terminal condition for name = absolute/relative path
  • Simplify logic; strchrnul will do exactly what we want
lib/libc/gen/posix_spawn.c
291

Does rfork_thread align the stack properly, or does it assume the caller did that?

lib/libc/gen/posix_spawn.c
291

On amd64, rfork_thread() loads the stack parameter into %rsp and then performs a call, so the stack will be properly aligned if stack is. Ensuring the stack size is a multiple of 16 and using aligned_alloc() will suffice.

However, on i386, rfork_thread() pushes 4 bytes onto stack and then performs a call, so doing the obvious thing will lead to a misaligned stack.

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

lib/libc/gen/posix_spawn.c
291

We don't actually provide or assume any better than 4-byte stack alignment on i386, this is a source of issues with GCC-compiled ports that I've brought up in the past to no avail.

In D25038#551763, @kib wrote:

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

I have mixed feelings about doing it in rfork_thread() vs. having rfork_thread() reject misaligned stack up-front with EINVAL and forcing the caller to be explicit about what they're doing. I don't know how rfork_thread gets used in practice outside of this one instance that it's used in base given that it's been discouraged for some years, but it feels somewhat wrong to move the stack pointer out from underneath the caller like this while not knowing anything about the shape/size of the stack or intent of the caller.

Adding an EINVAL return to rfork_thread will cause this code to fall through to the "use vfork instead" logic, hiding the error in unexpected ways.

I don't think rfork_thread should make any promises regarding the initial stackframe of the child beyond the fact of following the platform ABI for a function with a single void* parameter. So I would say that rfork_thread should do any required alignment.

While at it, would it be worth adding a check that the stack size is at least MINSIGSTKSZ + PATH_MAX ?

While at it, would it be worth adding a check that the stack size is at least MINSIGSTKSZ + PATH_MAX ?

When you say 'adding a check', are you proposing a static assert that PSPAWN_STACK_ALIGNMENT at least fits MINSIGSTKSZ + PATH_MAX , or runtime check just before calling rfork_thread()?

While at it, would it be worth adding a check that the stack size is at least MINSIGSTKSZ + PATH_MAX ?

When you say 'adding a check', are you proposing a static assert that PSPAWN_STACK_ALIGNMENT at least fits MINSIGSTKSZ + PATH_MAX , or runtime check just before calling rfork_thread()?

either a static assert that _RFORK_THREAD_STACK_SIZE is big enough, or simply define _RFORK_THREAD_STACK_SIZE as MINSIGSTKSZ + PATH_MAX aligned to the required alignment.

In D25038#551763, @kib wrote:

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

Does it have to be done in this patch, or can I follow-up after the imminent issue is fixed?

In D25038#551763, @kib wrote:

Ensure 2-byte alignment of the stack. For the non-spawnp case, we're using _RFORK_THREAD_STACK_SIZE without modifications so I've dropped a _Static_assert and comment near it to make it clear that any bumps should take into account the alignment requirements rather than bothering with any extra math for the easy case where it's fixed at compile-time anyways.

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

Does it have to be done in this patch, or can I follow-up after the imminent issue is fixed?

Have ? The issue is clearly unrelated, and even if you fixed it there, it needs a separate commit.

In D25038#551862, @kib wrote:
In D25038#551763, @kib wrote:

I think that alignment should be done in rfork_thread() instead, if any. So that it would be set for all users, not just this place.

Does it have to be done in this patch, or can I follow-up after the imminent issue is fixed?

Have ? The issue is clearly unrelated, and even if you fixed it there, it needs a separate commit.

Ah, sorry- the "instead, if any" chunk of your original comment lead me to believe that you'd prefer it get done here and now rather than after the fact.

What remains to be done to push this forward?

Will we be able to wrap this up before the conclusion of the 11.4 release cycle, or is this suddenly going to get bumped into SA status?

It's also not clear to me if we're now-satisfied with the changes, including stack alignment.

It can still be fixed before the release (if appropriate) with approval from so@. Otherwise, it will be a post-release follow-up item.

Will we be able to wrap this up before the conclusion of the 11.4 release cycle, or is this suddenly going to get bumped into SA status?

It's also not clear to me if we're now-satisfied with the changes, including stack alignment.

I've verified that as long as stack+stacksz is a multiple of 16 bytes, the child thread will have the expected stack alignment, so this looks right to me.

lib/libc/gen/exec.c
245

There's another bug here, though I don't think it's a security issue; cnt can be 0, which means that the memp array doesn't get terminated, and execve runs off the end of it and passes garbage args to the invoked script. Maybe add 3 rather than 2 to cnt above, and have:

    if (cnt > 0)
        bcopy(argv + 1, memp + 2, cnt * sizeof(char*));
    else
        memp[2] = NULL;
`
andrew_tao173.riddles.org.uk added inline comments.
lib/libc/gen/exec.c
214

This "continue" is wrong now that the advancement of the loop pointer has been moved to below it - it'll loop forever

This revision now requires changes to proceed.Jun 5 2020, 3:15 PM

Move advancement of op up to just after its use -- I suspect a previous iteration was still using op and I overlooked moving this up.

Fix the argv passed to execve(2) in the ENOEXEC fallback case and add some best-effort tests. argv[0] == NULL is not explicitly forbidden, so we should attempt to cope with it and at least terminate memp properly if we get it. The spec for execve seems to indicate that we should have been preserving argv[0] all along, but in the argv[0] == NULL edge-case we retain the old behavior of using "sh" because it'd be somewhat fragile otherwise. For reference:

In the cases where the other members of the exec family of functions would fail and set errno to [ENOEXEC], the execlp() and execvp() functions shall execute a command interpreter and the environment of the executed command shall be as if the process invoked the sh utility using execl() as follows:

execl(<shell path>, arg0, file, arg1, ..., (char *)0);

where <shell path> is an unspecified pathname for the sh utility, file is the process image file, and for execvp(), where arg0, arg1, and so on correspond to the values passed to execvp() in argv[0], argv[1], and so on.

I've conflated this with the pre-existing security isue since we need to allocate stack space for it anyways, and it seems like we should fix that detail as well. It could easily go in beforehand.

I think this addresses all of my points.

I'm going to take the silence as acceptance... secteam, can I please get approval to proceed to commit and MFC this all to the way to releng/11.4?

kevans retitled this revision from execvPe: obviate the need for environment-controlled stack allocation to execvPe: obviate the need for potentially large stack allocations.Jun 9 2020, 9:02 PM
kevans edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jun 9 2020, 9:21 PM

Per discussion with Kyle offline, we are going to defer this change until after the 11.4 release. We are just in a tough spot to inject a change of this size this late into the release process.

That said, I'm accepting the change (note, this is not a technical review) as secteam with the expectation that others (@kib, @jilles) will help review the latest patch and ensure we are good to go. (Which appears to already have happened.)

Thanks, folks. I'll aim to commit this tonight in two parts (ENOEXEC bug and then the rest) with an MFC after: 1 week

kevans changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 10 2020, 1:16 AM