Skip to content
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.

Sign up for GitHub

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

Write note about traps for capability stores #528

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Write note about traps for capability stores #528

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/riscv-integration.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,9 @@ capability written to memory is cleared if the authorizing capability does not
grant permission to write capabilities (i.e. both <<w_perm>> and <<c_perm>>
must be set in AP).

WARNING: #TODO: these cases may cause exceptions in the future - we need a way
for software to discover and/or control the behavior#
NOTE: Future extensions to {cheri_base_ext_name} may add mechanisms that cause
stores to raise exceptions when the authorizing capability does not grant both
<<w_perm>> and <<c_perm>>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs an “if the tag of the capability being stored to memory is set” somewhere?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add that to be extra clear but I since this is a note underneath "what happens when you don't have "C+W" I think it's probably fine to omit this clarification.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see nothing that states this any of this isn't for SC as a whole. The paragraph above just says the tag is cleared. C says "Allow reading capability data from memory if the authorizing capability also grants R-permission. Allow writing capability data to memory if the authorizing capability also grants W-permission." We don't define capability data anywhere, so a completely reasonable, if false, interpretation would be that any LC/SC counts as a load/store of capability data. 3.2.1 Tag talks about valid and invalid capabilities. I would in fact argue that capability data is both valid and invalid capabilities. Similarly, we have "Missing LM-permission does not affect untagged values since this could result in surprising bit patterns when copying non-capability data. Similarly, sealed capabilities are not modified as they are not directly dereferenceable." in various places, which is assuming a definition that only encompasses valid capabilities. This is all quite loose, making implicit assumptions and, I would argue, using the wrong definition.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine a future extension could have at least two options: 1) trap for tagged data missing W|C, 2) trap for any cap store missing W|C (even if that is less useful.

I do agree that we should make sure to be explicit about anything defined in the spec but since this is about some hypothetical future thing being vague until that extension actually exists seems ok?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well my point is the latter is something that should not be permitted, because it breaks memcpy

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, would you be willing to fix this note?


[#section_existing_riscv_insns]
=== Existing RISC-V Instructions
Expand Down
Loading