mmap PROT_CAP and PROT_NO_CAP support#23
Conversation
Support flags PROT_CAP and PROT_NO_CAP in mmap. PROT_CAP is implied by PROT_READ and PROT_WRITE for most mmap calls. PROT_NO_CAP forbids the use of capabilities in this mapping. Storing a tag in this mapping will cause a fault. Note that CHERI on riscv needs additional setting of CHERI_PERMS_LOAD_CAP or CHERI_PERMS_STORE_CAP, otherwise PROT_NO_CAP is not enforced. Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
Implement Zcheripte 0.9.3 tval2 encoding support for CHERI PTE faults. Return SEGV_STORETAG when there is a PTE fault. Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
Add selftests for mmap PROT_CAP and PROT_NO_CAP behavior. Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
| perms |= CHERI_PERMS_LOAD_CAP; | ||
| if (prot & PROT_WRITE) | ||
| perms |= CHERI_PERMS_STORE_CAP; | ||
| } |
There was a problem hiding this comment.
Sorry, but this and the related text in the commit message is bogus. I somewhat see why you are doing this but we really shouldn't add additional permissions to the capability with an explicitly given PROT_NO_CAP. Yes, you will get a CHERI fault instead of a capability fault but I don't think we care. If this fails tests adjust the tests.
There was a problem hiding this comment.
Note that it is even more bogus than that if the CW bit is not supported, either because the hardware does not have it or because for some reason the arch decided to not make use of it.
| return -EINVAL; | ||
| /* | ||
| * PROT_CAP is not supported with file-backed MAP_SHARED mapping | ||
| */ |
There was a problem hiding this comment.
Why is this? Isn't it the whole point to allow just that?
| local_flush_tlb_page(addr); | ||
| } | ||
|
|
||
| static inline bool access_error(unsigned long cause, struct vm_area_struct *vma) |
There was a problem hiding this comment.
I don't think this is quite right. First, you are introducing quite a large diff in a function with subtle logic. This is bound to introduce merge errors when the upstream code changes.
But more importantly, you are now saying that a cheri PTE fault on a VMA with VM_CAP_WRITE is ok. That will trigger standard fault handling which is then trying to fix an ordinary page fault and will fail to do so. So I think that all cheri PTE faults should be bad_area_nosemaphore. This also simpifies things because you can just handle those before even calling access_ok().
| @@ -351,11 +366,12 @@ void handle_page_fault(struct pt_regs *regs) | |||
| if (!vma) | |||
| goto lock_mmap; | |||
|
|
|||
There was a problem hiding this comment.
See above: Handle cheri_pte_faults here and always do an access error.
| perms |= CHERI_PERMS_LOAD_CAP; | ||
| if (prot & PROT_WRITE) | ||
| perms |= CHERI_PERMS_STORE_CAP; | ||
| } |
There was a problem hiding this comment.
Note that it is even more bogus than that if the CW bit is not supported, either because the hardware does not have it or because for some reason the arch decided to not make use of it.
| #define SEGV_CAPACCESSERR 14 /* Capability access fault */ | ||
| #define NSIGSEGV 14 | ||
| #define SEGV_STORETAG 15 /* Capability tag store fault */ | ||
| #define NSIGSEGV 15 |
There was a problem hiding this comment.
See the CI run: This breaks build on other archs.
chrehrhardt
left a comment
chrehrhardt
left a comment
There was a problem hiding this comment.
See inline comments. At the very least the arch_user_ptr_owning_perms_from_prot needs to go
|
This implementation doesn't appear to square with the design in CheriBSD https://man.cheribsd.org/cgi-bin/man.cgi/mmap. The key thing is that
In the original implementation (see CTSRD-CHERI/cheribsd#2191), I had It would be super helpful if @paul-metzger could update his cheribsdtest port and test this branch with it to see how things align. |
3 participants
In most cases PROT_CAP is automatically applied. PROT_NO_CAP removes the ability to store pointers and will create a segfault with SEGV_STORETAG.
This Pull Request also adds a self test, testing the assumptions of how it should work.
See the individual commit messages for more details.
Closes: #10