I think you can avoid the #ifdef in uvm_mmap.c by simply definining the macro PAX_MPROTECT_ADJUST() to return 0 if the feature is off.
Also, it would be wiser to just add error handling to the call in uvm_unix.c, rather then assuming it never fails. Or just remove the call there if it's so redundant - isn't the same check and adjustment actually done by uvm_mmap() later? I also see that after your change, pmap_mprotect_adjust() doesn't adjust the flags on error path any more - is that wise, what happens if caller ignores the EACCESS? I guess it would be safer to ensure the flags are sanitized regardless. Oh, and yes, I think EACCESS is nicer than EOPNOTSUPP, as the latter is more usually used for unsupported system calls rather then individual flags. I think actually EINVAL is more appropriate then either of those two, however - EACCESS seems to be more concerned with protection flags versus the descriptor mode rather than invalid flags. Jaromir 2016-12-26 20:11 GMT+01:00 Pierre Pronchery <[email protected]>: > Hi, > > I have simplified the patch, changed it to return EACCES upon errors, > adapted it to -current, and tested it there (both with PAX_MPROTECT set and > not set). It is still not 100% elegant though (adds an #ifdef) so I will > welcome ideas on how to improve it some more. > > Cheers, > -- khorben > > > On 26/12/2016 00:10, Pierre Pronchery wrote: >> >> On 10/12/2016 14:02, Michael van Elst wrote: >>> >>> [email protected] writes: >>> >>>> Why doesn't the following code get rejected by pax mprotect? >>> >>> >>>> a = mmap(NULL, BUFSIZ, PROT_READ | PROT_WRITE | PROT_EXEC, >>>> MAP_ANON, -1, 2); >>> >>> >>> It gets 'rejected' by silently dropping the PROT_EXEC flag. >> >> >> I find this awful: programs trying to use e.g JIT will fail to detect >> that it really is not supported, and crash later instead. >> >> I am attaching here a patch returning errors instead. >> >> Thanks to this patch, www/firefox works without having to set the "m: >> mprotect(2) restrictions, explicit disable" flag on its executable >> binaries (tested on netbsd-7/amd64). >> >>> POSIX would require mmap to fail with errno = EACCES. >> >> >> In the patch attached I have used ENOTSUP, because this is what OpenBSD >> seems to be using: >> http://man.openbsd.org/mmap.2 >> >> I also think EACCES (or EPERM?) would be better though, so I will be >> happy to replace it if considered more appropriate. >> >> I have changed the logic deciding which flags to drop. It used to be, >> independently of whether PROT_READ is set: >> - if PROT_WRITE, or PROT_WRITE and PROT_EXECUTE are set, then execution >> is silently denied; >> - otherwise, writing is silently denied. >> (which doesn't make much sense to me) >> >> Now there would be only one case instead: >> - if PROT_WRITE and PROT_EXECUTE are set, execution is denied and an >> error is returned. >> >> Another thing I will really need to know before committing this, is >> whether the changes should really be applied to sys_mmap() only. >> Finally, I left a XXX where there might be a side-effect, if applied in >> sys/kern/exec_subr.c, after calling vn_rdwr(). >> >> Cheers, > > > -- > khorben
