Re: [PATCH] Add Prefer_MAP_32BIT_EXEC for Silvermont

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Dec 11, 2015 at 9:02 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
> 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

I will give it a try.

> 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).

I will discuss it internally.

Thanks.

-- 
H.J.