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: Support page capability permissions #884

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

Conversation

brooksdavis
Copy link
Member

Add PROT_CAP_NONE, PROT_CAP_READ, and PROT_CAP_WRITE to manage capablity
permissions. To avoid the need to audit every mmap call (modifying
most), imply PROT_CAP_READ and PROT_CAP_WRITE if PROT_READ or PROT_WRITE
(respectively) are set unless at least one of PROT_CAP_* is set.

To permit expected or-ing behavior, PROT_CAP_NONE is overidden by
PROT_CAP_READ and PROT_CAP_WRITE when present. It must be stripped
when converting an mmap protection to a vm_prot_t.

@brooksdavis
Copy link
Member Author

Comments from @emaste reminded my I'd been thinking about doing this for a while. This needs tests and docs, but boots and passes current cheribsdtest tests. I'm looking for feedback on the approach.

Copy link
Member

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

The interaction with PROT_MAX looks like it's done correctly at a glance, though that's definitely something that warrants test coverage.

sys/vm/vm_mmap.c Show resolved Hide resolved
sys/vm/vm_mmap.c Outdated Show resolved Hide resolved
sys/vm/vm_mmap.c Outdated Show resolved Hide resolved
sys/sys/mman.h Outdated Show resolved Hide resolved
@brooksdavis brooksdavis force-pushed the mmap-cap-prot branch 2 times, most recently from dba5326 to c08f844 Compare January 14, 2021 20:16
Copy link
Collaborator

@bsdjhb bsdjhb left a comment

Choose a reason for hiding this comment

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

I think the overall approach seems ok. I do think for compat reasons it is hard to not imply CAP RW from PROT_RW unless explicitly overridden. It would be tempting to have some sort of PROT_CAP() macro that took PROT_READ/WRITE/NONE, but I think that wouldn't work. The only odd bit is that someone can write PROT_CAP_NONE | PROT_CAP_READ. I would be tempted to make that be an error (EINVAL) rather than letting PROT_CAP_READ override in that case.

sys/vm/vm.h Outdated Show resolved Hide resolved
sys/vm/vm_mmap.c Outdated Show resolved Hide resolved
sys/vm/vm_mmap.c Show resolved Hide resolved
@brooksdavis
Copy link
Member Author

brooksdavis commented Jan 19, 2021

The only odd bit is that someone can write PROT_CAP_NONE | PROT_CAP_READ. I would be tempted to make that be an error (EINVAL) rather than letting PROT_CAP_READ override in that case.

No allowing PROT_CAP_NONE | PROT_CAP_READ would be simplifying in some ways, but there's a decent bit of code OR's in additional permissions like:

perms |= PROT_CAP_READ;

and it would have to grow something like

perms &= ~ PROT_CAP_NONE;

That's not the worst thing, but seems easy to forget.

In my initial design, all three macros contained the PROT_CAP_NONE bit, but that seemed weird with VM_PROT_*_CAP being single bits.

@bsdjhb
Copy link
Collaborator

bsdjhb commented Jan 20, 2021

So the first design sounds more like a 'PROT_CAP_PRESENT' that then had a little bit field for 'READ' and 'WRITE'? That is probably the route I would have used with something like a PROT_CAP so you would do PROT_CAP(PROT_READ | PROT_WRITE) or PROT_CAP(PROT_NONE). That can get very verbose though: PROT_MAX(PROT_CAP(PROT_READ | PROT_WRITE) | PROT_READ | PROT_WRITE | PROT_EXECUTE) | PROT_CAP(PROT_READ | PROT_WRITE) | PROT_READ | PROT_WRITE. I guess no matter how you treat it you can't error for PROT_NONE | PROT_READ either since PROT_NONE is zero.

This text dates to the BSD 4.4 import and is misleading.  The
mprotect syscall acts on page granularity and breaks up mappings as
required to do so.  Anything less is not POSIX compliant.
This macro extracts the maximum protections either explicitly set via
PROT_MAX or implied by regular PROT_* flags in no maximum value is set.
Add PROT_CAP_NONE, PROT_CAP_READ, and PROT_CAP_WRITE to manage capablity
permissions.  To avoid the need to audit every mmap call (modifying
most), imply PROT_CAP_READ and PROT_CAP_WRITE if PROT_READ or PROT_WRITE
(respectively) are set unless at least one of PROT_CAP_* is set.

To permit expected or-ing behavior, PROT_CAP_NONE is overidden by
PROT_CAP_READ and PROT_CAP_WRITE when present.  It must be stripped
when converting an mmap protection to a vm_prot_t.
Consolidate the docuementation in mmap.2 rather than duplicating the
increasingly long description.
One simple test that explicit CAP_RW permissions match work just like
implied permissions.  Another that attempt to demonstrate tag stripping
when a page lacks CAP_READ and one the demonstrates faults storing
without CAP_WRITE.
@brooksdavis
Copy link
Member Author

Now that I've got basic working tests on MAP_ANON, I'm wondering how best to enumerate other types of backing objects, etc that should be tested.

@bsdjhb
Copy link
Collaborator

bsdjhb commented Jan 25, 2021

The only other backing stores currently are shm_open() and shm_attach()? I can't remember what we did for tmpfs, but if it does permit caps (which would require some sort of VFS flag or some such which is why I think we have it turned off for now), then we would perhaps use tmpfs fd's as a backing store. Probably attempts to use an explicit CAP prot other than PROT_CAP_NONE on backing stores that don't support tags (such as a UFS file) should fail with EINVAL or EOPNOTSUPP or some such (probably EINVAL).

@rwatson
Copy link
Member

rwatson commented Jan 25, 2021

I think by policy we should be supporting tags on capabilities only for anonymously mapped pages. This probably means MAP_ANON pages from mmap(2), COW derivations from other mapping types (e.g., COW of a vnode page, such as setting a non-BSS global variable to a capability value), shm_open(), and shm_attach(). I think we should not allow tags in tmpfs, which should maintain the same semantics as other filesystems .. at least until we've done research into tags and filesystems, which isn't a topic with an owner currently.

@arichardson
Copy link
Member

I don't think allowing tags on shm_open pages is a good idea since a capability written to the shared memory segment might not be mapped to the same underlying data in all processes.

@bsdjhb
Copy link
Collaborator

bsdjhb commented Jan 25, 2021

@arichardson I don't quite follow? shm_open() are just named versions of MAP_ANON (same as shm_attach()).

@rwatson It looks like we don't permit tags on tmpfs (no OBJ_HASCAP flag on the backing VM objects), but note that Linux processes on FreeBSD tend to use a tmpfs mounted at /dev/shm as the backing store for shm_open(), so we may in fact want a mount option in the future to permit tags when it is used in this fashion for Linux CheriABI on CheriBSD.

@bsdjhb
Copy link
Collaborator

bsdjhb commented Jan 25, 2021

@arichardson oh, the problem of sharing pointers between processes. This is true of any shared memory though including shm_attach() as well as shm_open(). I'm not sure we never want to allow passing pointers but it certainly does give you the ability to shoot your foot off and most of the time when dealing with shared memory you want to deal with offsets or ring indices, etc. and not actual pointers.

@rwatson
Copy link
Member

rwatson commented Jan 25, 2021

The main question in my mind has been whether we want an explicit indication that pointers should be shared either via the object, or via its mapping, when the object or mapping is created, rather than being "default on". But there are certainly real-world uses of multiple processes sharing pointers via shared memory. And, arguably, that's what we're doing in the co-process work as well.

@brooksdavis
Copy link
Member Author

Based on some discussion Tuesday, I think I want to alter this model to something slightly more complicated. For compatibility, we likely want to continue to imply capability load/store for MAP_PRIVATE mappings, but I think we should not do so for either MAP_SHARED or shmat where nearly all the time you don't want capabilities.

@paulmetzger paulmetzger deleted the mmap-cap-prot branch July 11, 2022 10:37
@jrtc27 jrtc27 restored the mmap-cap-prot branch July 11, 2022 14:54
@jrtc27 jrtc27 reopened this Jul 11, 2022
@brooksdavis brooksdavis marked this pull request as draft October 25, 2022 16:17
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.

6 participants