Skip to content

Conversation

nwf
Copy link
Member

@nwf nwf commented Jun 25, 2025

Changes sdk/core/loader/types.h to introduce a PermitLoadGlobal bit on import entries and teaches sdk/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.

@nwf nwf requested review from resistor and davidchisnall June 25, 2025 19:06
@davidchisnall
Copy link
Collaborator

davidchisnall commented Jun 26, 2025

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:

  • clang to expose these as attributes.
  • lld to generate the correct JSON for audit logs.
  • cheriot-audit to correctly understand the additional permissions.

This is not a change that can exist in the RTOS in isolation.

@nwf
Copy link
Member Author

nwf commented Jun 26, 2025

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.

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:

  • We would like, I think, to have caps with data-R/W and cap-W-only permissions to DMA engines, since there's no reason anyone should be reading back caps from the configuration registers.
    We can't express those, but SD MC !LM !LG might mitigate some of the gap?

  • The allocator's access to hazards was RD MC !SD !LM !LG under the old semantics and keeping that seemed slightly better than making it RD MC LG !SD !LM.

(The change in the allocator's check_gm is, I think, on second thought, wrong and I'm sort of surprised that it passed the test suite. Apologies, I was in a hurry yesterday.)

If this is the approach that we want to go with, we also need changes in:

  • clang to expose these as attributes.

Yes, that's the conflict mentioned.

  • lld to generate the correct JSON for audit logs.

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!).

  • cheriot-audit to correctly understand the additional permissions.

That I haven't yet done, but I can if we go this way.

This is not a change that can exist in the RTOS in isolation.

Acknowledged.

@nwf nwf marked this pull request as draft June 26, 2025 17:44
@davidchisnall
Copy link
Collaborator

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

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.

We can't express those, but SD MC !LM !LG might mitigate some of the gap?

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:

  • Remove C means that we know that all information leakage must be direct. We do this in the time shared object so that we know that the only information leak can be from the SNTP compartment to others, which means that there isn't a data flow (modulo timing covert channels) from time consumers to the network via NTP.
  • Removing W means that you can't write to it. If you have C, then removing m means that it's deeply immutable. In SNTP we make the shared object writeable only from the SNTP compartment and so we can ensure one-way dataflow.
  • Removing R means that you have write-only access, possibly with capabilities.

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)

nwf added a commit to nwf/llvm-project that referenced this pull request Jul 15, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
nwf added a commit to nwf/llvm-project that referenced this pull request Aug 6, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
nwf added a commit to nwf/llvm-project that referenced this pull request Aug 11, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 11, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 11, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 11, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
@nwf
Copy link
Member Author

nwf commented Aug 11, 2025

OK, CHERIoT-Platform/llvm-project#180 has landed, so now this is just blocked on a new devcontainer build.

resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 12, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 12, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 12, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 16, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 16, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 17, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 17, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 17, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 17, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 18, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
resistor pushed a commit to CHERIoT-Platform/llvm-project that referenced this pull request Aug 18, 2025
I'm stealing one of the heretofore unused flags for PermitLoadGlobal.
See CHERIoT-Platform/cheriot-rtos#547
@nwf nwf force-pushed the nwf/202506-import_load_globals branch 2 times, most recently from 4844b6d to c1be633 Compare August 18, 2025 19:49
@nwf nwf marked this pull request as ready for review August 18, 2025 19:49
@nwf nwf enabled auto-merge August 18, 2025 21:39
@davidchisnall
Copy link
Collaborator

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 __VA_OPT__ to provide a default.

@nwf nwf force-pushed the nwf/202506-import_load_globals branch from c1be633 to 67d5027 Compare August 22, 2025 15:57
@nwf
Copy link
Member Author

nwf commented Aug 22, 2025

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 __VA_OPT__ to provide a default.

C preprocessor crimes committed in the first commit; sensible changes made in the 2nd.

@nwf nwf force-pushed the nwf/202506-import_load_globals branch 2 times, most recently from fb9e8c0 to 0a1290e Compare August 28, 2025 18:41
@nwf
Copy link
Member Author

nwf commented Aug 28, 2025

To be more detailed: the C preprocessor crimes in the first commit move all the permissions parameters to __VA_ARGS__ and uses that to initialize a structure with a bool field for each permission. Because we need this to work in C and C++, we have to rely on common structure initialization semantics. Fortunately this all works out the way we want in both specifications:

  • C++ permits structs to be built from initializer lists with fewer elements than the structure (including empty initializer lists), list entries are interpreted as each sequential field, and elements not explicitly initialized in the list will be "copy-initialized from an empty initializer list", which amounts to zero initialization, and for the scalar bool type, that means the explicit conversion of 0, so false. (At least, all of that's true for C++11 and later.)

  • C permits structs to be built from (short) initializer lists (C23 even permits empty initializer lists, at long last), list entries are again interpreted as each sequential field, and elements not explicitly initialized are "empty initialized"; as we typedef bool to be C99's _Bool in <stdbool.h>, this means these (integral) fields initialize to 0, so false.

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 (false __VA_OPT__(|) __VA_ARGS__) ? ... : ... would not be.

@nwf nwf force-pushed the nwf/202506-import_load_globals branch from 0a1290e to d1ebe1e Compare August 28, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants