Re: [PATCH] Add Prefer_MAP_32BIT_EXEC for Silvermont

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 11-12-2015 14:40, H.J. Lu wrote:
> On Fri, Dec 11, 2015 at 8:23 AM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>> On 11-12-2015 13:59, H.J. Lu wrote:
>>> On Fri, Dec 11, 2015 at 7:39 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>>>>> On Fri, 11 Dec 2015, H.J. Lu wrote:
>>>>>
>>>>>>> +++ b/sysdeps/unix/sysv/linux/x86_64/64/mmap.c
>>>>>>> @@ -0,0 +1,49 @@
>>>>>>> +/* Copyright (C) 2015 Free Software Foundation, Inc.
>>>>>
>>>>> All new files should have a descriptive first line before the copyright
>>>>> notice.
>>>>>
>>> Here is the updated patch.
>>
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/mmap.c b/sysdeps/unix/sysv/linux/x86_64/64/mmap.c
>>> new file mode 100644
>>> index 0000000..c34f633
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/x86_64/64/mmap.c
>>
>>> +
>>> +__ptr_t
>>> +__mmap (__ptr_t addr, size_t len, int prot, int flags, int fd, off_t offset)
>>> +{
>>> +  /* If the Prefer_MAP_32BIT_EXEC bit is set, try to map executable pages
>>> +     with MAP_32BIT first.  */
>>> +  if (addr == NULL
>>> +      && (prot & PROT_EXEC) != 0
>>> +      && HAS_ARCH_FEATURE (Prefer_MAP_32BIT_EXEC))
>>> +    {
>>> +      addr = (__ptr_t) INLINE_SYSCALL (mmap, 6, addr, len, prot,
>>> +                                    flags | MAP_32BIT,
>>> +                                    fd, offset);
>>> +      if (addr != MAP_FAILED)
>>> +     return addr;
>>> +    }
>>> +  return (__ptr_t) INLINE_SYSCALL (mmap, 6, addr, len, prot, flags,
>>> +                                fd, offset);
>>> +}
>>> +
>>
>> I would advise not add another syscall variant implementation, but rather
>> work on make a generic implementation with adjustments made by each platform.
>> Something like
>>
>> __ptr_t
>> __mmap (__ptr_t addr, size_t len, int prot, int flags, int fd, off_t offset)
>> {
>>   flags = MMAP_ARCH_FLAGS (flags);
>>   return (__ptr_t) INLINE_SYSCALL( mmap, 6, addr, len, prot, flags, fd,  offset);
>> }
>>
>> And then define MMAP_ARCH_FLAG for x86 to add the MAP_32BIT when required.
>>
> 
> This won't work here since we fallback to the default mmap when
> MAP_32BIT fails.

So just change the MMAP_ARCH_FLAGS hook to something like:

 __ptr_t ret = __mmap_arch (addr, len, prot, flags, fd, offset));
 if (ret != MAP_FAILED)
   ret = INLINE_SYSCALL (mmap, 6, addr, len, prot, flags, fd, offset);
 return ret

The default value for __mmap_arch could be a constant value so compiler 
could remove the the comparison if it is the case.

Another issue is this is basically limiting ALSR really hard on x86_64.
I also would prefer to make the default to *not* include this flag and
set the env. variable to actually enable it. If the cpu is slow doing
what's intended because it is buggy, let it be slow at default. Do not
break what was intended (full ALSR).