Differential D21352
procctl(PROC_STACKGAP_CTL) Authored by kib on Aug 21 2019, 3:33 PM. Tags None Referenced Files
Subscribers
Details
Allows a process to request that stack gap was not applied to its stacks. This is actually of limited usefulness since the gap where the stack grows is still maintained, it just allows to completely exhaust the gap on per-process basis instead of setting global security.bsd.stack_guard_pages. [I will write the man pages updates after the code is agreed upon]
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions Just to make sure that we're on the same page, here is what I have inferred is happening based on that PR: First, the JVM is doing a mmap(MAP_STACK), which creates two vm map entries, a normal entry and beneath it a guard entry. Let X be the starting address of this guard entry. Then, the JVM is doing a mmap(X, "HotSpot Guard Size", MAP_ANON | MAP_FIXED), which creates the region labeled "HotSpot Guard Pages", replacing the initial part of the guard entry. Now, the "HostSpot Guard Pages" start at address X, and the starting address of the guard entry is address Y, where Y > X by the size of the "HotSpot Guard Pages". The trouble is vm_map_growstack() is still going to implement the security.bsd.stack_guard_page but at address Y rather than X. And, implementing it at Y interferes with the JVM's own guard mechanism. Yes? Comment Actions Almost, but not quite. There is pthread feature where thread can be created with some attributes. Among them are the stack size and guard size. We create the stack of the specified size as you described (two entries, guard has reserved part which cannot be grown into), and below the libthr.so puts yet another guard entry, of the size specified in the attribute. JVM hacks on that guard, not on the implicit kernel guard. I suspect that eliminating implicit kernel guard would fix JVM, but so far I was unable to receive the confirmation. Another suggestion I made was to do mmap(2) on a guard, and it seems that JVM already uses this, but again I am not sure. I probably need to spend several hours reading ktrace output from openjdk runs. Comment Actions Does libthr create a third, guard entry by default, or only if you pass an explicit "attr"?
I suspect that it does too. Comment Actions Default guard size is zero, so it is not created explicitly. Third is it because you count two guards and mapped part ? There is one detail, I forgot it, and think that it is mostly irrelevant, but it is still interesting. This was the reason why vm_map_protect() does clip and then skips GUARD entries. When creating a guarded stack, libthr does mmap(total size of stack and guard), then mprotect(guard). This clips out part of the guard to the permanent guard, the rest is the shrinking entry. Comment Actions Yes, that was how I arrived at 3. However, by default, i.e., without passing an "attr" that sets a non-zero guard size, there should only be 2 map entries, a guard entry and a normal entry for the "mapped part". Yes? Here is the code that the JVM uses for creating a thread: Note that the "attr" passed to pthread_create() doesn't include a guard size, so there shouldn't be a libthr-created guard.
Actually, I think that it is relevant. As far as I can tell, the explicitly created libthr guard doesn't disable the security.bsd.stack_guard_page guard. If so, we are actually killing the thread before it hits the libthr guard. Comment Actions Yes.
Well, it is relevant in the sense that the guard is created. I mean that it is irrelevant whether libthr does mprotect() or mmap(MAP_GUARD) to create the extra guard at the bottom. Comment Actions When a guard map entry is created the (object, offset) pair is (NULL, 0). Later, when that entry is clipped at the start, whether by mprotect() or mmap(MAP_FIXED), the offset for the remainder of the guard entry will be adjusted by the size of the clipping, even though the object is NULL. We could use the offset to recognize that an explicit guard exists, and automatically override the implicit security.bsd.stack_guard_page in vm_map_growstack(). Essentially, max_grow = gap_entry->end - gap_entry->start;
if (guard > gap_entry->offset &&
guard - gap_entry->offset > max_grow)
return (KERN_NO_SPACE);That said, I don't have an immediate answer to how this would handle the case of stacks that grow up. Perhaps, we would have to manually adjust the offset. Comment Actions I would also like to suggest that we store the stack_guard_page value that was in force at the time that the guard entry was created in the map entry and use this stored value in vm_map_growstack(). The next_read field should be unused by guard entries. Comment Actions This is something I will do definitely. For the proposal of automatic use of the offset for the guard entry to decrease the forced stack gap, I am not sure. First, it basically defeats the gap, since an appropriate mapping that is created in the place of the gap allows to clash the stack into it. The only protection left is that we now require MAP_FIXED to get something mapped into the gap area. Second, if doing that, I would prefer explicit checks and then explicit action of reduction of the stored gap size. That said, do you have any objections or suggestions for the procctl(2) knob ? Comment Actions I have no objections. I think that this is a feature that we should support. I only question whether disabling the stack gap on a process should apply retroactively to the existing stacks and not just the ones created after the procctl() operation. Comment Actions https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=239894#c17 Greg Lewis freebsd_committer 2019-08-30 22:58:04 UTC I got a VM set up with a recent -CURRENT, applied the patches from the review and rebuilt world. I can confirm that after that I can run proccontrol -m stackgap -s disable java -cp . InfiniteRecursion and get a StackOverflowError as expected. Running without the proccontrol line yields a crash as it did previously. I plan to commit this later today.
Comment Actions My only hesitation from "rubber stamping" this change is the difficulty of describing the complete semantics of this option, specifically, the semantics that arise when reenabling the stack gap. Suppose that we have disabled and then reenabled stack gaps for a process. Stacks created before gaps are disabled will then have their original gap enforced, unless the stack has grown into the previously defined gap region. Stacks created while gaps are disabled will have no gap after gaps are reenabled. Consequently, I want to ask: Should we actually support reenabling stack gaps on a process once they've been disabled? Comment Actions Split current program stackgap state control, and the state after execve. Current state can be only disabled, while state after exec can be re-enabled. Comment Actions Upload the right patch: Also update procctl(2) man page.
Comment Actions The code looks good.
Comment Actions Switch to interpret each flag in PROC_STACKGAP_CTL, instead of PROC_STACKGAP_ENABLE manage it all.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||