caprevoke fork fixes #1724

brooksdavis · 2023-06-22T22:57:59Z

Fix/work around some issues present in the caprevoke tree.

The first change is a cherry pick of a change that for reasons I don't fully understand triggers other issues: panic on Morello due to calling a NULL revocation test function and hang in rccorder (the change should cause a significant increase in the malloc/free calls as I believe the append pattern in the sh is basically realloc repeatedly.)
The first vmspace_fork() fix addresses the NULL pointer dereference.
The second I noticed while fixing the first.
Finally, I disable the rcorder use that causes hangs as it's quite hard to debug during boot. More investigation required.

@RhodiumToad

The local parts of rc already skip .sample files; we add .pkgsave to the list, and add logic for base. Thanks to @RhodiumToad for getting this started. Differential Revision: https://reviews.freebsd.org/D27962 Reviewed by: imp Pull Request: freebsd/freebsd-src#662 (cherry picked from commit 3693d91)

markjdb · 2023-06-23T17:54:55Z

sys/vm/vm_map.c

+			 * with a newly quarantined neighbor.
+			 */
+			if (old_entry == old_map->rev_entry)
+				new_map->rev_entry = new_entry;


vm_map_entry_quarantine() will compare the neighbours of new_entry with new_map->rev_entry to see if they can be merged with new_entry. So this assignment doesn't prevent merging from happening at the point where this entry is being added. It's not obvious to me that this is what we want, but I'm also not too familiar with the quarantine mechanism yet so could easily be missing something.

There's a further check in vm_map_mergeable_neighbors() (called from vm_map_try_merge_entries()) that will prevent the entry in rev_entry from being merged. That suggests the checks in vm_map_entry_quarantine() are redundant.

As background, we can't allow the entry pointed to by rev_entry to be merged (enlarged) because we may have already examined capabilities that were outside the current entry, but would be inside the new one and thus we'd fail to revoke those capabilities.

Got it, thank you.

brooksdavis · 2023-06-28T23:47:44Z

I've completely redone the core changes. I now believe the issue was that we weren't copying the CLG state to the new pmap in pmap_vmspace_copy(). By doing copying the state it aligns with the CLG in capdirty pages and thus we don't try to revoke on fault when we've never started a revocation in this particular process.

sys/vm/vm_cheri_revoke.c

sys/kern/kern_cheri_revoke.c

It was hard to tell what the if conditions were.

When the parent is GEN1, capdirty pages will also be GEN1, but now the child will be GEN0 so there will be a mismatch leading to faulted capdirty pages being revoked when revocation isn't in progress. If no revocation is triggered before the fault happens then vm_map.vm_cheri_revoke_test may be NULL leading to a panic.

sys/riscv/riscv/pmap.c

sys/arm64/include/pmap.h

nwf

Generally looks good to me; I'm sorry to have left you with such a mess. One comment about vmspace_fork, and one stylistic question, but even with those I think it's good to go as is.

sys/vm/vm_map.c

sys/riscv/riscv/pmap.c

This is a potentially expensive check we we might not want to keep it on by default even under invariants.

When called from exec_new_vmspace() to empty an unshared vmspace prior to exec, the vm_map will be initialized with a zeroed enqueue and dequeue epoch in the info page. When the process had previously revoked and we failed to reset the state and zero the kernel's notion of the epoch we ended up in an infinite loop were the kernel would see no work to do (passed start epoch in the past) and thus would not update the info page resulting in no progress being made. One could alternatively reset the state in exec_new_vmspace(), but the extra store seems harmless in the vm_map_free() case.

Verify that the empty vmspace is not revoking and has a zero epoch.

If the epoch is open then vm_cheri_revoke_test has been set and we need to copy it was that we can test capabilities when we come back to finish this epoch.

If the epoch is open we may be in the midst of a revocation and thus rev_entry may be set in old_map. In that case, set the corresponding entry in new_map.

Add a somewhat unprincipled set of __has_feature(capabilities) checks to allow cheri/revoke.h to be included unconditionally by libsysdecode's implementation.

Support the flags arguments of cheri_revoke(2) and cheri_revoke_get_shadow(2). Use compressed names (sysdecode_cr*()) as otherwise the formatting of the manpage is completely trashed (mandoc uses too much space between columns).

Add two new modes to cheribsdtest_cheri_revoke_lib_run which start or end a revocation of the entire array (rather than looping over it to process a random number of times (average should be)). Use this to open an epoch prior to forking and close it in the child process.

brooksdavis · 2023-07-05T20:12:49Z

I've fixed a lame hack in mktables in the libsysdecode commit after @jrtc27 pointed out I'd missed the exclude feature. Should be ready to land at this point (CI failures are out of disk space errors)

brooksdavis requested review from nwf and markjdb June 22, 2023 22:57

markjdb reviewed Jun 23, 2023

View reviewed changes

markjdb approved these changes Jun 23, 2023

View reviewed changes

Minor whitespace fixes

71a4aa0

brooksdavis force-pushed the caprevoke-fork-fixes branch from ac003a5 to 22d72dc Compare June 28, 2023 23:44

brooksdavis requested a review from markjdb June 28, 2023 23:47

jrtc27 reviewed Jun 28, 2023

View reviewed changes

sys/vm/vm_cheri_revoke.c Outdated Show resolved Hide resolved

sys/kern/kern_cheri_revoke.c Outdated Show resolved Hide resolved

brooksdavis force-pushed the caprevoke-fork-fixes branch from 22d72dc to 975ae66 Compare June 29, 2023 19:29

brooksdavis added 2 commits June 29, 2023 23:49

Add some comments to endifs for long blocks

dabd343

It was hard to tell what the if conditions were.

brooksdavis force-pushed the caprevoke-fork-fixes branch from 975ae66 to b290f6f Compare June 29, 2023 22:49

jrtc27 reviewed Jun 29, 2023

View reviewed changes

sys/riscv/riscv/pmap.c Outdated Show resolved Hide resolved

brooksdavis force-pushed the caprevoke-fork-fixes branch from b290f6f to 94775ff Compare June 30, 2023 00:07

nwf reviewed Jun 30, 2023

View reviewed changes

sys/arm64/include/pmap.h Show resolved Hide resolved

nwf reviewed Jun 30, 2023

View reviewed changes

sys/vm/vm_map.c Show resolved Hide resolved

brooksdavis force-pushed the caprevoke-fork-fixes branch from 94775ff to f93e1a7 Compare June 30, 2023 15:40

jrtc27 reviewed Jun 30, 2023

View reviewed changes

sys/riscv/riscv/pmap.c Outdated Show resolved Hide resolved

brooksdavis added 6 commits June 30, 2023 17:03

Verify consistency of CLG at the end of revocation

a16b34a

This is a potentially expensive check we we might not want to keep it on by default even under invariants.

vmspace_fork: assert CLG is consistent in new map

75d113c

exec_new_vmspace: assert revoker state is NONE

7b562e6

Verify that the empty vmspace is not revoking and has a zero epoch.

vmspace_fork: copy vm_cheri_revoke_test

5464555

If the epoch is open then vm_cheri_revoke_test has been set and we need to copy it was that we can test capabilities when we come back to finish this epoch.

vmspace_fork(): update rev_entry

eb2863c

If the epoch is open we may be in the midst of a revocation and thus rev_entry may be set in old_map. In that case, set the corresponding entry in new_map.

brooksdavis force-pushed the caprevoke-fork-fixes branch from f93e1a7 to eb2863c Compare June 30, 2023 17:36

brooksdavis mentioned this pull request Jun 30, 2023

caprevoke: additional tests needed around *fork/exec #1730

Closed

cheri/revoke.h: allow include on non-CHERI systems

ba69f53

Add a somewhat unprincipled set of __has_feature(capabilities) checks to allow cheri/revoke.h to be included unconditionally by libsysdecode's implementation.

brooksdavis force-pushed the caprevoke-fork-fixes branch from 3ae4fce to 352dc36 Compare June 30, 2023 23:44

brooksdavis force-pushed the caprevoke-fork-fixes branch from 352dc36 to b2db563 Compare July 5, 2023 17:55

brooksdavis added 4 commits July 5, 2023 20:55

libsysdecode: support caprevoke syscall flags

b886814

Support the flags arguments of cheri_revoke(2) and cheri_revoke_get_shadow(2). Use compressed names (sysdecode_cr*()) as otherwise the formatting of the manpage is completely trashed (mandoc uses too much space between columns).

kdump: decode cheri_revoke*(2) flags

44a46a9

truss: decode cheri_revoke*(2) flags

2bb6e98

brooksdavis force-pushed the caprevoke-fork-fixes branch from b2db563 to db2ac6a Compare July 5, 2023 19:55

brooksdavis merged commit da3f9d8 into caprevoke Jul 6, 2023
14 checks passed

brooksdavis deleted the caprevoke-fork-fixes branch July 6, 2023 01:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

caprevoke fork fixes #1724

caprevoke fork fixes #1724

brooksdavis commented Jun 22, 2023

markjdb Jun 23, 2023

brooksdavis Jun 23, 2023 •

edited

Loading

markjdb Jun 23, 2023

brooksdavis commented Jun 28, 2023

nwf left a comment

brooksdavis commented Jul 5, 2023

caprevoke fork fixes #1724

caprevoke fork fixes #1724

Conversation

brooksdavis commented Jun 22, 2023

markjdb Jun 23, 2023

Choose a reason for hiding this comment

brooksdavis Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

markjdb Jun 23, 2023

Choose a reason for hiding this comment

brooksdavis commented Jun 28, 2023

nwf left a comment

Choose a reason for hiding this comment

brooksdavis commented Jul 5, 2023

brooksdavis Jun 23, 2023 •

edited

Loading