-
Notifications
You must be signed in to change notification settings - Fork 57
Shared objects might want LoadGlobal permission #547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I don't think this is the correct approach. I believe not having load-global on shared objects that have RC permissions is a bug. We don't yet have a use case. My expectation is that you have Gl on all of these pointers (where l might be stripped if you lack R or C because they're dependent). The correct fix is to make sure that we don't lose those permissions, not to add more things. If this is the approach that we want to go with, we also need changes in:
This is not a change that can exist in the RTOS in isolation. |
I wasn't sure if the current behavior was deliberate, since the RTOS test suite carefully enumerates all the permissions expected on MMIO and shared object references. If you're happy for MC to imply LG on MMIO and shared object references, I could probably live with that until we have a strong use case otherwise. On that point, we surely don't have strong use cases yet, but:
(The change in the allocator's
Yes, that's the conflict mentioned.
That's CHERIoT-Platform/llvm-project#147 (which also has a fix to a field mask that we should take even if we don't take this approach, because we'll eventually want the linker and loader to agree on the field definition anyway!).
That I haven't yet done, but I can if we go this way.
Acknowledged. |
Just because I wrote some code and wrote some tests for the code doesn't mean that the code does what I meant. I think this is a good counterexample.
I think that's fine as a thing you might want to pass to DMA, the question is whether that's a desirable set of initial permissions for a pre-shared object. The existing permissions have some use cases:
I'm not sure what removing g means (it hasn't come up yet because we don't really use C for any normal uses - the allocator's hazard pointers are a special case with the write end mediated via the switcher). You can load pointers from the global, but you can't capture them. But that doesn't bound the time that you can hold them for in any way (a thread can load one, store it on its stack, and leave it there forever). The closest thing I can come to a use case is something where you have a broadcast work-queue-like thing and you want everyone do to an atomic exchange to remove items and then process them. Removing G and g after you've swapped a value out seems like a better idea there though. All of that said: If we make g implicit, removing it later is harder. This change doesn't change the semantics of any existing mappings that have reduced permissions, which is nice. And that makes me ponder if we should do it even if we don't know why yet. (Also, sorry, I wrote the previous comment before noticing the LLVM commit) |
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
OK, CHERIoT-Platform/llvm-project#180 has landed, so now this is just blocked on a new |
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
I'm stealing one of the heretofore unused flags for PermitLoadGlobal. See CHERIoT-Platform/cheriot-rtos#547
4844b6d
to
c1be633
Compare
Changing the number of arguments to the macros is an API-breaking change. I think we can avoid that by making it variadic, and using |
c1be633
to
67d5027
Compare
C preprocessor crimes committed in the first commit; sensible changes made in the 2nd. |
fb9e8c0
to
0a1290e
Compare
To be more detailed: the C preprocessor crimes in the first commit move all the permissions parameters to
I decided to go down this path because it means the macros are future compatible with some further additional permission flag, in ways that an alternative like |
0a1290e
to
d1ebe1e
Compare
Changes
sdk/core/loader/types.h
to introduce a PermitLoadGlobal bit on import entries and teachessdk/include/compartment-macros.h
to use it. The rest of the changes are catching up core components and the test suite (which is expanded a bit to test the new bit).This will not pass CI because the compiler currently mismatches the (existing) loader, in that it only masked out the top 4, not 8, bits of the length for types. Local testing with CHERIoT-Platform/llvm-project#180 suggests that at least that is simple to fix.
This surely conflicts with #545 and, unless @davidchisnall thinks I'm about to blow a hole in the security model, will require revisions to CHERIoT-Platform/llvm-project#147 .
Tagging @xdoardo who, for some reason, isn't eligible as a reviewer.