Fix handling of COW pages during revocation #1930
Conversation
pmap_caploadgen_update() previously checked PGA_WRITEABLE to decide whether the caller may mutate the returned page. However, this is not the right check: PGA_WRITEABLE is merely advisory and when set only means that there _may_ exist some writeable mapping of the page. Use the pmap's protections to tell upper layers what they should be doing. In particular, we must not revoke from a copy-on-write page.
If during a scan we find an unmapped-but-resident page, use state from the map entry to decide what to do. In particular, PGA_WRITEABLE is an advisory flag and shouldn't be used to determine whether a page may be modified in place.
bin/cheribsdtest/cheribsdtest_vm.c
Outdated
| /* | ||
| * Quarantine the third page. | ||
| */ | ||
| CHERIBSDTEST_CHECK_SYSCALL(cheri_revoke_get_shadow( |
There was a problem hiding this comment.
We can't really use these macros in a child process, they don't print anything. Does cheribsdtest have some facility to make it easier to get errors out of a child? If not I'll just use different exit statuses to make it easier to see what went wrong.
There was a problem hiding this comment.
Yeah, it shares that same misfeature with atf. Several existing tests do use these macros in a forked child, and probably don't work correctly if a system call fails in a child. It might be nice to have some *CHILD* variants that call exit directly with a non-zero error code?
There was a problem hiding this comment.
Yes, that'd be handy. For now I just did something low-tech.
There was a problem hiding this comment.
I encountered that the other day too, though it’s even worse for the child func case as ccsp is null so you crash rather than “just” set state that’s clobbered
markjdb
force-pushed
the
dev-caprevoke-cow
branch
from
3a1852f to
e306194
Compare
|
The riscv test failures are, I believe, the result of my test assuming particular behaviour of pmap_copy(), which on riscv is a no-op. |
bin/cheribsdtest/cheribsdtest_vm.c
Outdated
| * pmap. munmap() would have the same effect of course, but | ||
| * would also throw away the contents of the page. |
There was a problem hiding this comment.
Hmm, I guess I don't fully understand how munmap() would be different here. Is that munmap() would alter the behavior of the vm_map_entry in the child in a way? I guess maybe you want the revocation pass to scan both pages in the backing VM object, but the revocation pass limits itself to the subset of each VM object that is mapped by the vm_map_entry's? So if you did munmap() then the revoker would only attempt to scan the first page and not the second?
There was a problem hiding this comment.
We have two pages, each containing a capability referencing the third page. I want the revoker to visit both pages and revoke both of those capabilities. I'm trying to ensure that the revoker's pmap has a PTE for the second page but not the first. munmap() would destroy the PTE, but it would also create a hole in the vm_map, causing the revoker not to visit that page.
There was a problem hiding this comment.
... and that is really just a re-statement of what you wrote, yes. :)
There was a problem hiding this comment.
Might be worth expanding the comment a bit further then as I think as it stands now saying you want the page unmapped doesn't quite map to that, or at least unmapped is ambiguous. I think of unmapped primarily as "logically removed from the address space", i.e. munmap, but I think here you mean "removed from the pmap map (PTEs)". Just something about the vm_map_entry still being the same so the revoker scans it all, but getting the PTE cleared is probably sufficient?
There was a problem hiding this comment.
I reworded it to avoid mention of unmapping the page.
Use a three-page heap to verify two cases: one where a copy-on-write page bearing a to-be-revoked capability is mapped at the time that it is scanned, and one where the page is not mapped.
markjdb
force-pushed
the
dev-caprevoke-cow
branch
from
e306194 to
dcc912d
Compare
No description provided.