Re: [PATCH v7 1/4] support: Add support_stack_alloc

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 07/07/2021 14:15, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 07/07/2021 07:17, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> The code to allocate a stack from xsigstack is refactored so it can
>>>> be more generic.  The new support_stack_alloc() also set PROT_EXEC
>>>> if DEFAULT_STACK_PERMS has PF_X.  This is required on some
>>>>  architectures (hppa for instance) and trying to access the rtld
>>>> global from testsuite will require more intrusive refactoring
>>>> in the ldsodefs.h header.
>>>
>>> DEFAULT_STACK_PERMS is misnamed, it's really HISTORIC_STACK_PERMS.
>>> All architectures override it to RW permissions in the toolchain
>>> (maybe with the exception of Hurd, which uses trampolines for nested
>>> functions).
>>
>> This is in fact two different requirements, this gnulib thread gives
>> a nice summary about the permission required from trampolines [1]. 
>> Another requirement is how Linux layout the signal return code for the 
>> signal handler stack.  It seems that hppa still requires executable 
>> stacks, since tst-xsigstack does fails without a executable stack even 
>> on a recent 5.10.46-1 kernel.
> 
> Ugh, okay.
> 
>>> I have a cstack_allocate version that handles this.  It can only be done
>>> from within glibc proper because we do not export the stack execution
>>> status directly.  But I think it's out of scope for glibc 2.34 by now.
>>
>> We can in theory access the ldsodes.h fields directly and then
>> use GL (dl_stack_flags) information to set the stack executable or not.
>> The problem is ldsodefs.h is quite convoluted and it would require more
>> refactoring to use outside libc.so code.  But I agree with you that
>> having less hacky way to obtain this information is better.
>>
>> So are you ok with the current approach or being conservative and use
>> DEFAULT_STACK_PERMS on libsupport?
> 
> DEFAULT_STACK_PERMS with a comment is fine.

I have added:

  /* Some architecture still requires executable stack for the signal return                                     
     trampoline, although PF_X could be overridden if PT_GNU_STACK is present.                                        
     However since there is glibc does not export such information with a                                        
     proper ABI, it uses the historical permissions.  */  

> 
> I will resubmit my cstack_allocate patches for glibc 2.35 patches, and
> they will fully handle executable stacks.

I think once we get cstack_allocate we might use on 'support_stack_alloc'
instead.

> 
>>>> +  /* The guard bands need to be large enough to intercept offset
>>>> +     accesses from a stack address that might otherwise hit another
>>>> +     mapping.  Make them at least twice as big as the stack itself, to
>>>> +     defend against an offset by the entire size of a large
>>>> +     stack-allocated array.  The minimum is 1MiB, which is arbitrarily
>>>> +     chosen to be larger than any "typical" wild pointer offset.
>>>> +     Again, no matter what the number is, round it up to a whole
>>>> +     number of pages.  */
>>>> +  size_t guardsize = roundup (MAX (2 * stacksize, 1024 * 1024), pagesize);
>>>> +  size_t alloc_size = guardsize + stacksize + guardsize;
>>>> +  /* Use MAP_NORESERVE so that RAM will not be wasted on the guard
>>>> +     bands; touch all the pages of the actual stack before returning,
>>>> +     so we know they are allocated.  */
>>>> +  void *alloc_base = xmmap (0,
>>>> +                            alloc_size,
>>>> +                            PROT_NONE,
>>>> +                            MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE|MAP_STACK,
>>>> +                            -1);
>>>> +  /* PF_X can be overridden if PT_GNU_STACK is present.  */
>>>> +  int prot = PROT_READ | PROT_WRITE
>>>> +	     | (DEFAULT_STACK_PERMS & PF_X ? PROT_EXEC : 0);
>>>> +  xmprotect (alloc_base + guardsize, stacksize, prot);
>>>> +  memset (alloc_base + guardsize, 0xA5, stacksize);
>>>> +  return (struct support_stack) { alloc_base + guardsize, stacksize, guardsize };
>>>
>>> This doesn't handle different stack growth directions.
>>>
>>
>> At least for the usages of the routine it does not require any adjustment:
>> xsigaltstack and xclone will handle it.  I saw no regression for
>> tst-xsigaltstack and tst-clone_range.
> 
> Huh, I would expect the guard area to be outside of the stack region
> returned by stack allocation.  That's how the cstack_allocate API does
> it.  If the current tests expect something else, then the approach in
> the patch is okay (with a comment for DEFAULT_STACK_PERMS).

It seems that at least for hppa, sigaltstack already handles it.  For clone(),
xclone does the adjustment explicitly.