Page MenuHomeFreeBSD

vm_map_protect: allow to set prot and max_prot in one go.
ClosedPublic

Authored by kib on Tue, Jan 12, 12:48 PM.

Details

Summary

Also enable setting wx for max_prot even if map does not allow to set effective wx protection.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Tue, Jan 12, 12:48 PM
kib created this revision.
sys/vm/vm_map.c
2914

Suppose one initially has prot = maxprot = PROT_READ|PROT_WRITE, and we use mprotect() to set prot = PROT_READ|PROT_WRITE and maxprot = PROT_READ. From my reading, this code will leave the entry in a situation where prot & ~maxprot != 0.

sys/vm/vm_map.c
2914

Wouldn't be such attempt rejected by the added check at the start of the function, right before 'again' label ?

sys/vm/vm_map.c
2914

No, because it checks the new prot against the old maxprot.

sys/vm/vm_map.c
2914

Oh sorry, you're right, I was looking at the check in the first loop.

This revision is now accepted and ready to land.Tue, Jan 12, 8:54 PM

Use another mach error code to translate to ENOTSUP for maxprot>prot violation check. Existing selection collided with EINVAL, breaking several tests, e.g. largepage mprotect.

This revision now requires review to proceed.Tue, Jan 12, 9:32 PM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jan 12, 11:37 PM
This revision was automatically updated to reflect the committed changes.
sys/vm/vm_map.c
2839–2840

Aren't these two lines pointless? Two "if" statements later we have:

if ((flags & VM_MAP_PROTECT_SET_PROT) == 0 ||
    ...)
        continue;
2841–2842

This also appears pointless. Specifically, I don't see new_maxprot used by the rest of this loop body.

sys/vm/vm_map.c
2841–2842

I tried to be protective. While it is easy to see that the last loop, that finally changes protection settings of the map entries, only access new_prot and new_maxprot when guarded by corresponding flags, for the first and second loop it is not that obvious (and for first loop it is not true).

So instead of micro-optimizing, I just added the safety belts. If you prefer the following to be committed, I will do it.

diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index 4bd0b245a881..b3288fce5114 100644
--- a/sys/vm/vm_map.c
+++ b/sys/vm/vm_map.c
@@ -2836,11 +2836,6 @@ vm_map_protect(vm_map_t map, vm_offset_t start, vm_offset_t end,
 			return (rv);
 		}
 
-		if ((flags & VM_MAP_PROTECT_SET_PROT) == 0)
-			new_prot = entry->protection;
-		if ((flags & VM_MAP_PROTECT_SET_MAXPROT) == 0)
-			new_maxprot = entry->max_protection;
-
 		if ((flags & VM_MAP_PROTECT_SET_PROT) == 0 ||
 		    ((new_prot & ~entry->protection) & VM_PROT_WRITE) == 0 ||
 		    ENTRY_CHARGED(entry) ||
sys/vm/vm_map.c
2841–2842

Please do commit it.