Page MenuHomeFreeBSD
Differential D21081

Make randomized stack gap between strings and pointers to argv/envs.
ClosedPublic
Actions

Authored by kib on Jul 26 2019, 9:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 3:18 PM
Unknown Object (File)
Tue, Nov 12, 5:24 AM
Unknown Object (File)
Tue, Nov 12, 5:02 AM
Unknown Object (File)
Sep 24 2024, 7:28 AM
Unknown Object (File)
Sep 24 2024, 6:45 AM
Unknown Object (File)
Sep 11 2024, 11:38 AM
Unknown Object (File)
Sep 5 2024, 9:57 AM
Unknown Object (File)
Aug 18 2024, 5:11 PM
View All 71 Files
Subscribers
akumar3_isilon.com
cem
delphij
gordon
imp

Details

Reviewers
emaste
markj
brooks
cem
Commits
rS350484: Make randomized stack gap between strings and pointers to argv/envs.
Summary

This effectively makes the stack base on the csu _start entry randomized.

The gap is enabled if aslr is for the ABI is enabled, and then kern.elf{64,32}.aslr.stack_gap specify the max percentage of the initial stack size that can be wasted for gap. Setting it to zero disables the gap, and max is capped at 50%.

Test Plan

I used the following to print the %rsp/%esp value:

/* $Id: print_sp.c,v 1.2 2019/07/26 21:12:13 kostik Exp kostik $ */

#include <stdio.h>

int
main(void)
{
	unsigned long sp;

#ifdef _LP64
	__asm volatile("movq\t%%rsp,%0" : "=r" (sp));
#else
	__asm volatile("movl\t%%esp,%0" : "=r" (sp));
#endif
	printf("sp %#lx\n", sp);
}

Also I checked that args and environment is correctly passed.

Diff Detail

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

Event Timeline

kib created this revision.Jul 26 2019, 9:31 PM
Herald added a subscriber: imp. · View Herald TranscriptJul 26 2019, 9:31 PM
Harbormaster completed remote builds in B25525: Diff 60177.Jul 26 2019, 9:31 PM
kib edited the test plan for this revision. (Show Details)Jul 26 2019, 9:32 PM
kib updated this revision to Diff 60179.Jul 26 2019, 9:45 PM

Move the gap above auxv, so that the auxv initialization in static libc sees expected layout.

Harbormaster completed remote builds in B25527: Diff 60179.Jul 26 2019, 9:45 PM
kib updated this revision to Diff 60181.Jul 26 2019, 9:48 PM

Remove unused define.

Harbormaster completed remote builds in B25528: Diff 60181.Jul 26 2019, 9:48 PM
markj added inline comments.Jul 29 2019, 3:01 PM
sys/kern/imgact_elf.c
162

The description indicates that this is a boolean value, but it is a percentage.

emaste added inline comments.Jul 29 2019, 5:37 PM
sys/kern/imgact_elf.c
162

The default value 1 (in this review) also suggests a boolean.

2744

We should probably bring arc4random_uniform from userland to libkern and use it here, perhaps later.

emaste added subscribers: gordon, delphij, cem.Jul 29 2019, 5:38 PM
cem added inline comments.Jul 29 2019, 6:10 PM
sys/kern/imgact_elf.c
2742–2743

When eff_stack_sz is not very large relative to 100, this seems likely to result in more error than first multiplying by the percentage.

Do we anticipate the ULONG_MAX / 50 < eff_stack_sz condition ever?

If not, I think range = eff_stack_sz * pct / 100; makes more sense.

2744

Totally. I made an attempt before (r346410) but libkern and libc have object name conflicts, so it was reverted. The historical pattern is to copy files into both places :-(.

kib marked 2 inline comments as done.Jul 29 2019, 8:20 PM
kib added inline comments.
sys/kern/imgact_elf.c
162

Changed to 3 for more amusement.

2742–2743

Well, in the expression x / y * z, there is no requirements on the order of evaluation imposed by std, since this expression does not have any sequence points. Writing the calculation as two full expressions does impose the order.

In fact I do not think this is very important there, it is up to user to specify a reasonable value for gap pct, and more, the range 1-50 is probably reasonable always.

kib updated this revision to Diff 60250.Jul 29 2019, 8:26 PM
kib marked an inline comment as done.

Update sysctl description.
Rewrite the formula.

Harbormaster completed remote builds in B25576: Diff 60250.Jul 29 2019, 8:26 PM
cem accepted this revision.Jul 29 2019, 10:15 PM
cem added inline comments.
sys/kern/imgact_elf.c
2742–2743

there is no requirements on the order of evaluation imposed by std

I don't think this is true; the standard defines left-associativity via the syntax.
There is an explicit counter-example in C17/18 §5.1.2.3, 15 "Example 6":

EXAMPLE 6 To illustrate the grouping behavior of expressions, in the following fragment

int a, b;
/* ... */
a = a + 32760 + b + 5;

the expression statement behaves exactly the same as

a = (((a + 32760) + b) + 5);

due to the associativity and precedence of these operators. Thus, the result of the sum (a + 32760) is next added to b , and

that result is then added to 5 which results in the value assigned to a . On a machine in which overflows produce an explicit
trap and in which the range of values representable by an int is [−32768, +32767], the implementation cannot rewrite this
expression as

a = ((a + b) + 32765);

...

and more generally it seems rewriting from left-associative is only allowed if the compiler can know in advance that the computational result is identical.

This revision is now accepted and ready to land.Jul 29 2019, 10:15 PM
markj added inline comments.Jul 31 2019, 3:02 PM
sys/kern/imgact_elf.c
163

Maybe "maximum percentage of main stack to waste on a random gap"?

kib marked an inline comment as done.Jul 31 2019, 7:07 PM
kib added inline comments.
sys/kern/imgact_elf.c
163

Thanks, I used the wording literally. Will update the review if any further changes are requested.

markj added inline comments.Jul 31 2019, 7:45 PM
sys/kern/imgact_elf.c
2746

Don't we need 16 byte alignment on amd64 at least?

kib marked an inline comment as done.Jul 31 2019, 8:10 PM
kib added inline comments.
sys/kern/imgact_elf.c
2746

This is not the bottom of the stack, the gap is between strings and the rest (auxv, envp, argv, argc). The final alignment is performed by e.g. sys/amd64/amd64/machdep.c:exec_setregs() which aligns the stack according to the ABI requirements after all the data was copied.

markj accepted this revision.Jul 31 2019, 8:13 PM
Closed by commit rS350484: Make randomized stack gap between strings and pointers to argv/envs. (authored by kib). · Explain WhyJul 31 2019, 8:23 PM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rS350484: Make randomized stack gap between strings and pointers to argv/envs..
akumar3_isilon.com added a subscriber: akumar3_isilon.com.Oct 12 2021, 3:12 PM

Revision Contents
Changeset List

PathSize
sys/
amd64/
amd64/
1 line
compat/
freebsd32/
3 lines
ia32/
1 line
kern/
26 lines
4 lines
sys/
1 line
1 line
1 line

Diff 60181

View Options

sys/amd64/amd64/elf_machdep.c

Loading...
View Options

sys/compat/freebsd32/freebsd32_misc.c

Loading...
View Options

sys/compat/ia32/ia32_sysvec.c

Loading...
View Options

sys/kern/imgact_elf.c

Loading...
View Options

sys/kern/kern_exec.c

Loading...
View Options

sys/sys/imgact.h

Loading...
View Options

sys/sys/imgact_elf.h

Loading...
View Options

sys/sys/sysent.h

Loading...