Re: W^X mmap

[Jaromír Doleček <jaromir.dolecek%gmail.com@localhost>]

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 <khorben%defora.org@localhost>:
>                         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:
>>>
>>> coypu%SDF.ORG@localhost 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