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.

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

mmap: explicitly expose capability permissions #2191

Merged
merged 3 commits into from
Sep 13, 2024
Merged

mmap: explicitly expose capability permissions #2191

merged 3 commits into from
Sep 13, 2024

Conversation

brooksdavis
Copy link
Member

Introduce three new PROT_ values PROT_READ_CAP, PROT_WRITE_CAP, and PROT_NO_IMPLY_CAP. They combine to allow capability permissions to be implied in unmodified code using PROT_READ and PROT_WRITE allowing capability read and write permissions to be set or unset explicity.

If any of PROT_READ_CAP, PROT_WRITE_CAP, or PROT_NO_IMPLY_CAP are set, then the values of the PROT_READ_CAP and PROT_WRITE_CAP flag bits define the page protections and capability permissions for a given mapping.

In the underlying implementation, PROT_READ_CAP and PROT_WRITE_CAP map to VM_PROT_READ_CAP and VM_PROT_WRITE_CAP respectively and PROT_NO_IMPLY_CAP maps to a new VM_PROT_NO_IMPLY_CAP. VM_PROT_NO_IMPLY_CAP used transiently in fo_mmap implementations to avoid accidently adding capability permission and is also added to vm_entry's max_protection to allow superset tests to succeed when reducing capability permissions on a mapping via mmap or mprotect.

This differs from the approach in #884 in that PROT_NO_IMPLY_CAP is carried down as VM_PROT_NO_IMPLY_CAP so that VM_PROT_ADD_CAP doesn't add capabilities it shouldn't. The one implementation thing that's a bit wonky is that you have to add VM_PROT_NO_IMPLY_CAP to protections before you starting doing subset comparisons. I've mostly solved this by adding VM_PROT_NO_IMPLY_CAP to max_protection in the vm entry (at that point you're definitely done implying capability permissions).

bin/cheribsdtest/cheribsdtest_vm.c Outdated Show resolved Hide resolved
bin/cheribsdtest/cheribsdtest_vm.c Outdated Show resolved Hide resolved
bin/cheribsdtest/cheribsdtest_vm.c Outdated Show resolved Hide resolved
mmap_check_bad_protections(PROT_WRITE_CAP, ENOTSUP);
mmap_check_bad_protections(PROT_MAX(PROT_READ_CAP), ENOTSUP);
mmap_check_bad_protections(PROT_MAX(PROT_WRITE_CAP), ENOTSUP);
mmap_check_bad_protections(PROT_READ | PROT_WRITE | PROT_READ_CAP,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, on current arches this should be encodable in PTEs. Is this in anticipation of using a single 'C' permission bit in PTEs in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

My general view is that it's of limited use (I can't think of any use cases) and both future PTE and capability permission encodings may render it impossible. By forbidding it now we ensure that no one relies on it as I'd prefer not to end up in a situation were have to fail open due to running code that tries to do this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, I would argue for my last comment which is to just have a single PROT_CAP.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a commit (to be squashed before merge) which combines them. This is a little awkward PROT_ and VM_PROT flags no longer match, but it is simplifying.

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 if this is the direction we want, we might want to merge the VM_PROT_*CAP flags as well, but I'm not sure of that yet and how that interacts with revocation (e.g. might there still be a case where you want to do a COW-like thing where you only allow cap loads but not cap stores or vice versa).

lib/libsys/mmap.2 Outdated Show resolved Hide resolved
lib/libsys/mmap.2 Outdated Show resolved Hide resolved
lib/libsys/mmap.2 Outdated Show resolved Hide resolved
sys/vm/vm_map.c Show resolved Hide resolved
sys/vm/vm_mmap.c Outdated Show resolved Hide resolved
lib/libsys/mmap.2 Outdated Show resolved Hide resolved
lib/libsys/mmap.2 Outdated Show resolved Hide resolved
lib/libsys/mmap.2 Show resolved Hide resolved
@brooksdavis brooksdavis force-pushed the mmap-cap-prot3 branch 2 times, most recently from 33b49a7 to 31a8f5a Compare August 28, 2024 17:51
Use _PROT_ALL instead of ORing all the flags.
Refactor VM_PROT_ADD_CAP() macro to use a statement expression and if
statements.  Eliminates multiple expansion of the prot argument and
prepares for future changes allowing explicit capablity permission
selection.
Introduce two new PROT_ values PROT_CAP and PROT_NO_CAP.  They combine
to allow capability permissions to be implied in unmodified code using
PROT_READ and PROT_WRITE which allowing capability permissions to be set
or unset explicity.

If either of PROT_CAP or PROT_NO_CAP are set, then the value of
the PROT_CAP flag bit defines the page protections and capability
permissions for a given mapping.

In the underlying implementation, PROT_CAP maps to VM_PROT_READ_CAP and
VM_PROT_WRITE_CAP depending on the values of PROT_READ and PROT_WRITE.
PROT_NO_CAP maps to a new VM_PROT_NO_IMPLY_CAP.  VM_PROT_NO_IMPLY_CAP
is used transiently in fo_mmap implementations to avoid accidently
adding capability permission and is also added to vm_entry's
max_protection to allow superset tests to succeed when reducing
capability permissions on a mapping via mmap or mprotect.
@brooksdavis brooksdavis added the ready-to-land PR is ready to land after revisions label Sep 12, 2024
@bsdjhb bsdjhb merged commit d2f37a3 into dev Sep 13, 2024
29 checks passed
@bsdjhb bsdjhb deleted the mmap-cap-prot3 branch September 13, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-land PR is ready to land after revisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants