cheribsd icon indicating copy to clipboard operation
cheribsd copied to clipboard

Add cheribsdtest_vm_tag_mprotect_none_and_back

Open nwf opened this issue 3 years ago • 3 comments

I believe this test should pass, but at present it does not. It appears the page is left without capability read permission after the second mprotect call:

root@:/ # cheribsdtest-purecap cheribsdtest_vm_tag_mprotect_none_and_back                                  
TEST: cheribsdtest_vm_tag_mprotect_none_and_back: check that mmap/mprotect(NONE)/mprotect(RW) still permits cap RW
FAIL: cheribsdtest_vm_tag_mprotect_none_and_back: 'cheri_gettag(arena[1])' is FALSE!

It's possible that this is because of the "funny" nature of tag-clearing PTEs: because they don't trap, they must be eagerly corrected unlike other permission upgrades (e.g., pages springing into existence or becoming writeable).

nwf avatar Jun 30 '21 01:06 nwf

This is failing due to the logic at https://github.com/CTSRD-CHERI/cheribsd/blob/4ea9e6bc33debc22355747d79866ef3c92549bb4/sys/vm/vm_map.c#L3037-L3043

After mprotect(PROT_NONE), entry->max_protection retains VM_PROT_READ_CAP|VM_PROT_WRITE_CAP but entry->protection is 0. The subsequent mprotect(PROT_READ|PROT_WRITE) then fails to VM_PROT_ADD_CAP because set_max is false, so we only look to entry->protection for justification, and we find none.

I'm not quite sure what the right move is, and so am quite open to suggestion. Perhaps we shouldn't VM_PROT_ADD_CAP(new_prot) here, which always copies VM_PROT_READ to VM_PROT_READ_CAP, but should rather add VM_PROT_READ_CAP to new_prot if both new_prot & VM_PROT_READ and entry->max_protection & VM_PROT_READ_CAP (and similarly for VM_PROT_WRITE_CAP).

There's room to add PROT_READ_CAP and PROT_WRITE_CAP to the sys/sys/mman.h definitions, too, so we could expose those explicitly to userland, too, but that seems like a deeper change.

nwf avatar Jul 23 '21 00:07 nwf

There's room to add PROT_READ_CAP and PROT_WRITE_CAP to the sys/sys/mman.h definitions, too, so we could expose those explicitly to userland, too, but that seems like a deeper change.

That's #884 :)

jrtc27 avatar Jul 23 '21 01:07 jrtc27

There's room to add PROT_READ_CAP and PROT_WRITE_CAP to the sys/sys/mman.h definitions, too, so we could expose those explicitly to userland, too, but that seems like a deeper change.

That's #884 :)

Unfortunately #884 needs a complete rewrite. I realized I was inferring cap permissions inappropriately and haven't figured out the right model yet.

brooksdavis avatar Jul 28 '21 17:07 brooksdavis