Details

Reviewers

mw
emaste
kib
imp
cy

Commits

rG16a900ae0242: setrlimit: Take stack gap into account.
rG889b56c8cd84: setrlimit: Take stack gap into account.

Summary

Calling setrlimit with stack gap enabled and with low values of stack
resource limit often caused the program to abort immediately after
exiting the syscall. This happened due to the fact that the resource
limit was calculated assuming that the stack started at sv_usrstack,
while with stack gap enabled the stack is moved by a random number
of bytes.

Save information about stack size in struct vmspace and adjust the
rlim_cur value. If the rlim_cur and stack gap is bigger than rlim_max,
then the value is truncated to rlim_max.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dgr_semihalf.com requested review of this revision.Aug 12 2021, 8:31 AM

dgr_semihalf.com created this revision.

kd added a subscriber: kd.Aug 13 2021, 2:45 PM

cy added a subscriber: cy.Aug 16 2021, 5:12 AM

I have reverted (locally) the NTP mitigation commits. This patch totally resolves the NTP issues. The code looks good. Good to go from this perspective.

Secondly, firefox still segfaults (unless procctl (proccontrol -m aslr -s disable firefox) is used. I think this might be a juxtaposed issue.

This revision is now accepted and ready to land.Aug 16 2021, 5:02 PM

In D31516#711576, @cy wrote:

I have reverted (locally) the NTP mitigation commits. This patch totally resolves the NTP issues. The code looks good. Good to go from this perspective.

Secondly, firefox still segfaults (unless procctl (proccontrol -m aslr -s disable firefox) is used. I think this might be a juxtaposed issue.

Yeah, firefox still segfaults. Both the firefox issue and ntpd issue are caused by the fact that the kernel assumes that stack starts at a constant address. This is not the case with stack gap anymore. There are more places in which the kernel assumes that stack starts at a constant address, and that's why firefox still doesn't work. I'll send a patch which should fix the rest of the issues this week.

Sorry, but I do not agree with this approach. We have to start stack at USRSTACK (and I had to provide knob to shrink LA57 to LA48 in part due to this issue), and IMO we should keep the traditional interpretation of RLIMIT_STACK. In particular, RLIMIT_STACK use by programs that to mprotect(MLOCK_ALL) is to limit the amount of wired stack memory.

You can try something less intrusive if you want to make this programs work without elf note flag, for instance you could auto-adjust soft RLIMIT_STACK to not be less than gap + new value, but still in hard limit of RLIMIT_STACK. The vm_stkgap part of your patch would be reused, with a small addition in kern_resource.c. But I would not do even that: just set a flag.

Updated setrlimit to automatically adjust stack rlim_cur value by the stack gap size. This also works, however now the rlim_max is properly respected. Ntpd also doesn't crash.

This revision now requires review to proceed.Aug 26 2021, 1:54 PM

dgr_semihalf.com mentioned this in D31692: [RFC] Adjust kern.usrstack sysctl by stack gap..Aug 26 2021, 2:04 PM

dgr_semihalf.com added a child revision: D31692: [RFC] Adjust kern.usrstack sysctl by stack gap..

2khramtsov_gmail.com added a subscriber: 2khramtsov_gmail.com.Aug 28 2021, 9:03 PM

Hi @kib,

Do you have any remarks about this version of the diff (and also D31692)?

Personally I don't think that disabling stack gap for every misbehaving program is the right approach.

kib added inline comments.Sep 2 2021, 8:26 PM

sys/kern/imgact_elf.c
2699	I suggest to move the assignment from there to exec_stackgap(). Both to have the set up/cleaning in the same place, and to make vm_stkgap initialization non depended on specific image activator.
sys/kern/kern_exec.c
1574–1575	Make sv_stackgap method return the gap value, and assign it to vm_stkgap there.
sys/kern/kern_resource.c
675	This needs to be clamped against hard limit.
sys/vm/vm_map.h
296	We have vm_size_t type. Also please note in the comment that the gap size is in bytes.

Made sv_stackgap return the size of the stack gap.
Moved vm_stkgap assignment into exec_stackgap().
Changed vm_stkgap type to vm_size_t
Modified a comment to mention that stack gap size is in bytes.

dgr_semihalf.com added a child revision: D31897: kern_exec: Add kern.stacktop sysctl..Sep 10 2021, 6:20 AM

Ping. Any remarks?

Hi, do you have any further comments or remarks? If there are no objections, I'm going to merge this patch by EOW.

kib added inline comments.Oct 13 2021, 10:59 AM

sys/kern/kern_exec.c
1575	I suggest to add local struct proc *p var, initialized with imgp->proc. Then you can simplify at least three lines in the function.

Added struct proc *p helper variable.

kib added inline comments.Oct 13 2021, 11:11 PM

sys/kern/kern_resource.c
676	You are clamping against the hard limit passed by userspace? This is wrong. You need to clamp it against process rlim_max. Hmm, I think this is already done at line 692. I am sorry for not noting it initially.

Removed the clamping.

dgr_semihalf.com added inline comments.Oct 14 2021, 9:23 AM

sys/kern/kern_resource.c
676	Yeah, looking at this more closely, this is already being done for every resource limit slightly later in this function, so we don't need it here.

kib accepted this revision.Oct 14 2021, 2:26 PM

This revision is now accepted and ready to land.Oct 14 2021, 2:26 PM

Closed by commit rG889b56c8cd84: setrlimit: Take stack gap into account. (authored by dgr_semihalf.com, committed by mw). · Explain WhyOct 15 2021, 8:23 AM

This revision was automatically updated to reflect the committed changes.

mw added a commit: rG889b56c8cd84: setrlimit: Take stack gap into account..

markj added a subscriber: markj.Nov 18 2021, 5:05 PM

markj added inline comments.

sys/kern/kern_resource.c
693	If userspace does `rlim_cur = rlim_max = <value>`, which IMHO is reasonable, then the stack gap is not taken into account, and the vm_map_protect() call below can clobber the existing stack if the gap is big enough. Why don't we adjust the hard limit by the gap as well?

dgr_semihalf.com added inline comments.Nov 18 2021, 5:23 PM

sys/kern/kern_resource.c
693	The original patch did something similar (although it didn't adjust the rlimit structure, so it was invisible for the user). I adjusted this to be less intrusive according to kib's suggestion.

kib added inline comments.Nov 18 2021, 5:28 PM

sys/kern/kern_resource.c
693	We must not allow user to increase hard limit, unless user has appropriate privilege. Then, this automatic tweaking should not depend on the user uid. If there is an issue with vm_map_protect() over-extending its region, it should be clamped accordingly.

mw added a commit: rG16a900ae0242: setrlimit: Take stack gap into account..Dec 30 2021, 3:37 PM

setrlimit: Take stack gap into account.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 94939

sys/kern/imgact_elf.c

sys/kern/kern_exec.c

sys/kern/kern_resource.c

sys/sys/imgact_elf.h

sys/sys/sysent.h

sys/vm/vm_map.h

sys/vm/vm_map.c

setrlimit: Take stack gap into account.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 94939

sys/kern/imgact_elf.c

sys/kern/kern_exec.c

sys/kern/kern_resource.c

sys/sys/imgact_elf.h

sys/sys/sysent.h

sys/vm/vm_map.h

sys/vm/vm_map.c

setrlimit: Take stack gap into account.
ClosedPublic
Actions

Revision Contents
Changeset List