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

[Adhemerval Zanella Netto]

On 01/09/25 07:52, Yury Khrustalev wrote:
> 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.
> 
> Why? This tests checks how pthread_create works, we should call the
> pthread_create from libc.

The xpthread_create issues pthread_create, it is a wrapper to avoid adding
the error check boilerplate.

> 
>>> +  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).
> 
> I'll have a look at parsing memory mappings, but this is internal
> target-specific test that checks target-specific stuff, so I don't see
> why access internal struct is an issue here.

It could be a tests-internal, but I think avoid poking internal glibc metadata
should make the test more robust. 
> 
>>> +  if (pthread_cancel (th))
>>> +    {
>>> +      printf ("cannot cancel thread\n");
>>> +      TEST_VERIFY (false);
>>> +      return;
>>> +    }
>>> +  else
>>> +
>>>
>>> ...
>>>
>>> +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);
> 
> Strange, I don't see this error. How can I reproduce it? What compiler
> and / or configure command are you using?

$ aarch64-glibc-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=/home//azanella/toolchain/install/compilers/aarch64-linux-gnu/bin/aarch64-glibc-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/home/azanella/toolchain/install/compilers/15/aarch64-linux-gnu/bin/../libexec/gcc/aarch64-glibc-linux-gnu/15.1.1/lto-wrapper
Target: aarch64-glibc-linux-gnu
Configured with: /home/azanella/toolchain/src/gcc/configure --prefix=/home/azanella/toolchain/install/compilers/aarch64-linux-gnu --build=aarch64-unknown-linux-gnu --host=aarch64-unknown-linux-gnu --target=aarch64-glibc-linux-gnu --with-sysroot=/home/azanella/toolchain/install/compilers/aarch64-linux-gnu/sysroot --enable-initfini-array --disable-libssp --disable-libcilkrts --with-gmp=/home/azanella/toolchain/install/host-libraries --with-mpfr=/home/azanella/toolchain/install/host-libraries --with-mpc=/home/azanella/toolchain/install/host-libraries --disable-libsanitizer --disable-libstdcxx-pch --enable-languages=c,c++ --enable-shared --enable-threads
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 15.1.1 20250722 [master r16-2421-g65f044a3ef6] (GCC)

And the glibc configured with --enable-stack-protector=all --enable-bind-now=yes --enable-profile=yes 
--enable-fortify-source=2 --enable-hardcoded-path-in-tests --enable-memory-tagging.

> 
>> 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.
> 
> I'll have a look. I think it's better to check the result of write here.

The nptl/tst-guard1.c does check the result of a write on a memory region
marked as read-only, but it uses sigsetjmp to avoid UB in the signal handler.

> 
>>> ...
>>>
>>> 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
>>>
>>> ...
>>>
>>> +  pthread_t th;
>>> +  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).
> 
> I'll see how this test can be improved. It is important to verify that
> no lingering shadow stack allocations remain in case when
> pthread_create failed.

Yes, but different than tst-gcs-clone3-cancel.c where we can make it an
internal test; relying in UB make the test brittle.

I think a /proc/self/smaps parsing routine to compare before and after the
pthread calls can be used in both tests to check if the guard control region
was correctly deallocated.

> 
>>> ...
>>>
>>> +#include <support/test-driver.c>
>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3.c b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3.c
>>> new file mode 100644
>>> index 0000000000..3a1c596f9d
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/tst-gcs-clone3.c
>>>
>>> ...
>>>
>>> +  /* 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.
> 
> Good point, will fix.
> 
> Thanks,
> Yury
>