Kernel+DynamicLoader: Various fixes around mmap & mprotect syscalls #24190
Conversation
github-actions
bot
added
the
👀 pr-needs-review
supercomputer7
force-pushed
the
fix-mmap-prot-validation
branch
from
92a5909
to
6b38f6f
Compare
|
Really like this change from userspace perspective. I think I spend around 20 minutes the day before yesterday figuring out how and when various protections get enabled. |
supercomputer7
force-pushed
the
fix-mmap-prot-validation
branch
2 times, most recently
from
a58a0f4
to
96a2f9c
Compare
|
The second commit will probably disable |
No, if it had done that, the CI would have failed. It does, however, prevent loading modules with text relocations (since only them require remapping from W to X memory) but DynamicLoader doesn't really like text relocations even in initially loaded objects. |
|
This has a conflict with DanShaders' Kernel/Memory/Region changes that I merged today |
ADKaster
added
⚠️ pr-has-conflicts
This flag is set only once, and should never reset once it has been set, making it an ideal SetOnce use-case. It also simplifies the expected conditions for the enabling prctl call, as we don't expect a boolean flag, but rather the specific prctl option will always set (enable) Process' AddressSpace syscall region enforcing.
We add a prctl option which would be called once after the dynamic loader has finished to do text relocations before calling the actual program entry point. This change makes it much more obvious when we are allowed to change a region protection access from being writable to executable. The dynamic loader should be able to do this, but after a certain point it is obvious that such mechanism should be disabled.
Before of this change, actually setting the m_access to contain the
HasBeen{Readeable,Writable,Executable} bits was done by the method of
Region set_access_bit which added ORing with (access << 4) when enabling
a certain access bit to achieve this.
Now this is changed and when calling set_{readeable,writable,executable}
methods, they will set an appropriate SetOnce flag that could be checked
later.
supercomputer7
force-pushed
the
fix-mmap-prot-validation
branch
from
96a2f9c
to
39a02d2
Compare
supercomputer7
removed
the
⚠️ pr-has-conflicts
github-actions
bot
added
the
👀 pr-needs-review
Right, it will prevent loading modules that need text relocations to work. Not that we do any of that in our codebase, so it should be OK. |
github-actions
bot
removed
the
👀 pr-needs-review
This PR should fix a couple of issues I've seen in current implementation:
DynamicLoaderhas to do some text relocation and to properly support this, we do various checks (which are "fine"), but we might as well want to completely avoid such complication in the first place.Regionhas_been_{r,w,x}flags are not clearly understandable because nothing sets them clearly. A simple search doesn't show those bits being set, and only by looking deeper into theRegionset_access_bitmethod we could see that there's ORing with(access << 4)when enabling an access bit.The following 3 commits are trying to solve all of the mentioned problems. Hopefully I didn't mess anything up, but I'd like to get reviews on these changes and be pointed on problems if so are appeared in this PR.