Fix the is_procstack expression in vm_map_growstack(...).
AbandonedPublic
Actions

Authored by op on Jun 30 2015, 1:19 PM.

Details

Reviewers

kib
jhb
rwatson

Summary

Since the shared-page exists in some systems, this expression is no
longer correct. Fix them by adding the check for upper bound too.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

op updated this revision to Diff 6578.Jun 30 2015, 1:19 PM

op retitled this revision from to Fix the is_procstack expression in vm_map_growstack(...)..

op updated this object.

op edited the test plan for this revision. (Show Details)

op added reviewers: kib, jhb, rwatson.

op set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a subscriber: imp. · View Herald TranscriptJun 30 2015, 1:19 PM

This is not correct. vm_ssize is the current stack size, not the total stack size. The unmodified condition wants to know if the faulted address belongs to the current process main thread' stack as if the stack has grown up to its limits.

Patch makes the test to check the strange condition instead which cannot be reasonably explained. It sets is_procstack to true if faulted address is not farther from the bottom of the stack than the current stack size is.

What you want to do is probably best expressed as addr >= vm_vm_maxsaddr && addr < p->p_sysent->sv_usrstack, assuming this would not break some weird ABI emulator.

This revision now requires changes to proceed.Jun 30 2015, 1:55 PM

Sure, I'll update the patch in short times.

Updated, as Konstantin requested / mentioned.

This is exactly what I suggested.

The big question is, does something break due to the change ? It should work for the FreeBSD ABI binaries, but other ABI emulators must be reviewed to have some minimal confidence that they do not break. I do not think it is feasible to require testing them, might be Linuxolator only ?

sys/vm/vm_map.c
3643–3644	Excessive (). Next line as well.

No, I use them on the proper stack randomization in HardenedBSD. The NetBSD's and OpenBSD's version are just a gap based stack randomization, but I implemented the proper randomization in HardenedBSD.
http://jenkins.hardenedbsd.org/~op/true-stack-random.txt
http://jenkins.hardenedbsd.org/~op/firefox.procstat

Updated.

In D2954#57998, @op wrote:

No, I use them on the proper stack randomization in HardenedBSD. The NetBSD's and OpenBSD's version are just a gap based stack randomization, but I implemented the proper randomization in HardenedBSD.

I do not understand the answer. So did you inspected the ABI emulators' code and ensured that they are not affected by the change ? They should, but there could be some surprises.

How the HardenedBSD thing is related to my question, I do not understand. I want to avoid breaking existing in-tree FreeBSD code by this commit.

http://jenkins.hardenedbsd.org/~op/true-stack-random.txt
http://jenkins.hardenedbsd.org/~op/firefox.procstat

I actually tested your patch with ibcs2 binary and with a.out binary. Would be ideal to test a linux binary as well, but from the code inspection linuxolator does not care.