[PATCH v2 3/3] aarch64: GCS: add clone3 tests for shadow stack

[Adhemerval Zanella Netto]

On 08/09/25 12:33, Yury Khrustalev wrote:
> * Adhemerval
> 
> On Tue, Aug 26, 2025 at 04:46:01PM -0300, Adhemerval Zanella Netto wrote:
>>
>> On 07/07/25 09:47, Yury Khrustalev wrote:
>>> If GCS is available check that new thread is created with a
>>> shadow stack allocated by Glibc.
>>>
>>> Check that shadow stack is de-allocated if a new thread hasn't
>>> started or has been cancelled.
>>>
>>> ...
>>>  
>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-cancel.c b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-cancel.c
>>> new file mode 100644
>>> index 0000000000..b2a0a66abb
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-cancel.c
>>>
>>> ...
>>>
>>> +static void
>>> +test_cancel (void)
>>> +{
>>> +  pthread_t th;
>>> +  if (pthread_create (&th, NULL, forever, NULL) != 0)
>>> +    {
>>> +      perror ("pthread_create");
>>> +      TEST_VERIFY (false);
>>> +      return;
>>> +    }
>>
>> Maybe use xpthread_create.
> 
> Will fix in the next version of the patch (along with xpthread_cancel
> and xpthread_join pointed elsewhere).
> 
>>> +  printf ("thread created\n");
>>> +  struct pthread *pd = (struct pthread *)th;
>>> +  void *token = pd->shadow_stack_token;
>>
>> I am not sure if accessing an internal member field is the best way
>> here.  I think maybe it would be better to work by parsing /proc/self/smaps
>> and check for only one mapping with 'ss' in VmFlags (the main thread).
> 
> Parsing /proc/self/smaps requires much more code, I'll add it in the
> next version of the patch. After the thread started we'll have 2
> mappings with with the ss flag: parent and child. After cancellation we
> expect 1 mapping (the parent one).

It depends if the kernel supports MADV_GUARD_INSTALL and if the pthread
requires a guard page.  I think to simplify you can disable the guard page
to always expect one mapping.

> 
> If generic enough, we probably should put it somewhere in the `support`
> folder?

Yes, the support is the place to such routines.

> 
>>>
>>> ...
>>>
>>> +#include <support/test-driver.c>
>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-helper.h b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-helper.h
>>> new file mode 100644
>>> index 0000000000..4ca36aa5b3
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-helper.h
>>>
>>> ...
>>>
>>> +#ifndef TST_GCS_CLONE3_HELPER_H
>>> +#define TST_GCS_CLONE3_HELPER_H
>>> +
>>> +#include <setjmp.h>
>>> +
>>> +jmp_buf jmp;
>>> +
>>> +static void
>>> +handler (int signum)
>>> +{
>>> +  TEST_VERIFY (signum == SIGSEGV);
>>> +  write (STDOUT_FILENO, "in signal handler\n", 18);
>>
>> I am seeing:
>>
>> ../sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3.c: In function ‘handler’:
>> ../sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3.c:74:3: error: ignoring return value of ‘write’ declared with attribute ‘warn_unused_result’ [-Werror=unused-result]
>>    74 |   write (STDOUT_FILENO, "in signal handler\n", 18);
>>
>> The TEST_VERIFY will eventually call printf, so I suggest to use something
>> similar I did on nptl/tst-guard1.c to check if the page is writable.
> 
> Thanks for the configure flags you provided in another thread. I'll fix
> this by checking the result return from the write() function.
> 
>>>
>>> ...
>>>
>>> +#endif // TST_GCS_CLONE3_HELPER_H
>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-nomem.c b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-nomem.c
>>> new file mode 100644
>>> index 0000000000..07d381ed29
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3-nomem.c
>>>
>>> ...
>>>
>>> +  if (pthread_create (&th, NULL, fun, NULL) != 0)
>>> +    {
>>> +      perror ("expected: pthread_create");
>>> +      setrlimit (RLIMIT_AS, &prev);
>>> +      struct pthread *pd = (struct pthread *)th;
>>> +      void *token = pd->shadow_stack_token;
>>> +      pthread_join (th, NULL);
>>
>> It is UB to call pthread_join on an invalid handler, and I do not think
>> we should add a test that rely on an specific implementation detail
>> (where pthread_create might return a ligering value in the handler).
> 
> You are correct, (x)pthread_join() should be outside of this if branch
> (for the case when thread has in fact started). Will fix in the new
> version of the patch.
> 
>>>
>>> ...
>>>
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3.c
>>>
>>> ...
>>>
>>> +  if (base == 0)
>>> +    {
>>> +      puts ("shadow stack was not allocated by Glibc");
>>> +      exit (EXIT_UNSUPPORTED);
>>> +    }
>>> +  printf ("shadow stack top:     %016lx\n", top);
>>> +  printf ("shadow stack token:   %016lx\n", token);
>>> +
>>> +  /* This macro guard is for the sake of compilers that don't
>>> +     yet have the __builtin_aarch64_gcspr() builtin so that
>>> +     the test could compiler regardless.  */
>>> +#if __ARM_FEATURE_GCS_DEFAULT
>>
>> This fails for compilers that do not define __ARM_FEATURE_GCS_DEFAULT. I think
>> you meant 'ifdef' here.
> 
> Correct. Will fix this too.
> 
> Thanks,
> Yury
>