diff mbox series

[2/2] nptl: Enable MTE for stacks created for threads.

Message ID 20250314132142.49243-3-cupertino.miranda@oracle.com (mailing list archive)
State Changes Requested
Delegated to: Adhemerval Zanella Netto
Headers
Series RFC: Support for MTE stack tagging. |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent

Commit Message

Cupertino Miranda March 14, 2025, 1:21 p.m. UTC 
  This patch adds MPROT_MTE flag to newly mapped stacks for threads, if
DT_AARCH64_MEMTAG_STACK is present in the array of dynamic tags in the
executable.

---
 elf/dl-mte-stack.c                             |  1 +
 nptl/allocatestack.c                           |  3 ++-
 nptl/nptl-stack.c                              |  7 +++++++
 nptl/nptl-stack.h                              |  2 ++
 sysdeps/unix/sysv/linux/aarch64/Makefile       |  5 +++++
 sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c | 11 ++++++++++-
 6 files changed, 27 insertions(+), 2 deletions(-)

Comments

Florian Weimer March 14, 2025, 4:35 p.m. UTC | #1 
* Cupertino Miranda:

> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 800ca89720..6155157063 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -153,7 +153,8 @@ static int allocate_stack_mode = ALLOCATE_GUARD_MADV_GUARD;
>  static inline int stack_prot (void)
>  {
>    return (PROT_READ | PROT_WRITE
> -	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
> +	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0)
> +	  | stack_mem_tagging_prot ());
>  }

I think we should clean this up and arrange for the dynamic linker to
determine the PROT_* flags for stacks created at run time, so that we
can use that variable directly without further processing.

Thanks,
Florian
Adhemerval Zanella Netto March 19, 2025, 7:35 p.m. UTC | #2 
On 14/03/25 10:21, Cupertino Miranda wrote:
> This patch adds MPROT_MTE flag to newly mapped stacks for threads, if
> DT_AARCH64_MEMTAG_STACK is present in the array of dynamic tags in the
> executable.
> 
> ---
>  elf/dl-mte-stack.c                             |  1 +
>  nptl/allocatestack.c                           |  3 ++-
>  nptl/nptl-stack.c                              |  7 +++++++
>  nptl/nptl-stack.h                              |  2 ++
>  sysdeps/unix/sysv/linux/aarch64/Makefile       |  5 +++++
>  sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c | 11 ++++++++++-
>  6 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/dl-mte-stack.c b/elf/dl-mte-stack.c
> index f0063b54a0..f5b8293c7a 100644
> --- a/elf/dl-mte-stack.c
> +++ b/elf/dl-mte-stack.c
> @@ -25,3 +25,4 @@ _dl_mte_stack_protect (void *stack_end)
>    return ENOSYS;
>  }
>  rtld_hidden_def (_dl_mte_stack_protect)
> +
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 800ca89720..6155157063 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -153,7 +153,8 @@ static int allocate_stack_mode = ALLOCATE_GUARD_MADV_GUARD;
>  static inline int stack_prot (void)
>  {
>    return (PROT_READ | PROT_WRITE
> -	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
> +	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0)
> +	  | stack_mem_tagging_prot ());
>  }
>  
>  static void *
> diff --git a/nptl/nptl-stack.c b/nptl/nptl-stack.c
> index c049c5133c..34fbba1cd1 100644
> --- a/nptl/nptl-stack.c
> +++ b/nptl/nptl-stack.c
> @@ -145,3 +145,10 @@ __pthread_get_minstack (const pthread_attr_t *attr)
>  	  + PTHREAD_STACK_MIN);
>  }
>  libc_hidden_def (__pthread_get_minstack)
> +
> +int
> +__stack_mem_tagging_prot (void)
> +{
> +  return 0;
> +}
> +weak_alias (__stack_mem_tagging_prot, stack_mem_tagging_prot)

As indicated, this causas the linknamespace issues because stack_mem_tagging_prot
is in the application namespace.

> diff --git a/nptl/nptl-stack.h b/nptl/nptl-stack.h
> index ac672e16cf..2bfcaf7252 100644
> --- a/nptl/nptl-stack.h
> +++ b/nptl/nptl-stack.h
> @@ -61,4 +61,6 @@ __nptl_tls_static_size_for_stack (void)
>    return roundup (GLRO (dl_tls_static_size), GLRO (dl_tls_static_align));
>  }
>  
> +int stack_mem_tagging_prot (void);
> +
>  #endif /* _NPTL_STACK_H */
> diff --git a/sysdeps/unix/sysv/linux/aarch64/Makefile b/sysdeps/unix/sysv/linux/aarch64/Makefile
> index 8fb0e65b2a..e91a756836 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/Makefile
> +++ b/sysdeps/unix/sysv/linux/aarch64/Makefile
> @@ -10,6 +10,11 @@ sysdep-dl-routines += \
>    dl-mte-stack
>  endif
>  
> +ifeq ($(subdir),nptl)
> +routines += \
> +  dl-mte-stack
> +endif
> +

This add a static linking issue (link-static-libc test) with multiple
definition of _dl_mte_stack_protect and stack_mem_tagging_prot.  

And I don't think you need to set the MTE mode per thread, once the
mode it sets either by the tunable of by DT_AARCH64_MEMTAG_MODE it
should not be changed by glibc.  

I also think it should be done with Florian's suggestion, so there
is no need to add teh __stack_mem_tagging_prot hook and override it
on aarch64.

Now that we don't change dl_stack_flags during process execution,
we can more to the _rtld_global_ro and setup the mmap flags after
loader setup instead of the ELF ones.

>  ifeq ($(subdir),stdlib)
>  gen-as-const-headers += ucontext_i.sym
>  endif
> diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c b/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c
> index a4bc82c99b..7ed92855ae 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c
> @@ -42,7 +42,7 @@ void
>  _dl_mte_stack_protect (void *stack_end)
>  {
>    if ((GLRO(dl_aarch64_cpu_features).mte_state & AARCH64_CPU_FEATURE_MTE_STATE_STACK) == 0
> -      ||  !_dl_mte_mode ())
> +      || !_dl_mte_mode ())
>      return;
>  
>    int errval = 0;
> @@ -72,3 +72,12 @@ cannot set stack with PROT_MTE");
>      }
>  }
>  
> +int
> +stack_mem_tagging_prot (void)
> +{
> +  if ((GLRO(dl_aarch64_cpu_features).mte_state & AARCH64_CPU_FEATURE_MTE_STATE_STACK) == 8
> +      && _dl_mte_mode ())
> +    return PROT_MTE;
> +  else
> +    return 0;
> +}
Cupertino Miranda March 26, 2025, 5:26 p.m. UTC | #3 
Hi Florian,

Thanks for the review!

On 14-03-2025 16:35, Florian Weimer wrote:
> * Cupertino Miranda:
> 
>> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
>> index 800ca89720..6155157063 100644
>> --- a/nptl/allocatestack.c
>> +++ b/nptl/allocatestack.c
>> @@ -153,7 +153,8 @@ static int allocate_stack_mode = ALLOCATE_GUARD_MADV_GUARD;
>>   static inline int stack_prot (void)
>>   {
>>     return (PROT_READ | PROT_WRITE
>> -	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
>> +	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0)
>> +	  | stack_mem_tagging_prot ());
>>   }
> 
> I think we should clean this up and arrange for the dynamic linker to
> determine the PROT_* flags for stacks created at run time, so that we
> can use that variable directly without further processing.
IMHO, we cannot escape from abstracting somehow the final flags as above 
with PF_X.
The problem being that each OS would have each own flags and syscalls 
and dynamic linker is OS agnostic, AFAIK.
Were you suggesting to create a PF_MTE and abstract it that way ?
Or having dl_stack_flags to immediately be set to PROT_MTE, etc?

Thanks,
Cupertino

> 
> Thanks,
> Florian
>
Florian Weimer March 26, 2025, 6:44 p.m. UTC | #4 
* Cupertino Miranda:

> Hi Florian,
>
> Thanks for the review!
>
> On 14-03-2025 16:35, Florian Weimer wrote:
>> * Cupertino Miranda:
>> 
>>> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
>>> index 800ca89720..6155157063 100644
>>> --- a/nptl/allocatestack.c
>>> +++ b/nptl/allocatestack.c
>>> @@ -153,7 +153,8 @@ static int allocate_stack_mode = ALLOCATE_GUARD_MADV_GUARD;
>>>   static inline int stack_prot (void)
>>>   {
>>>     return (PROT_READ | PROT_WRITE
>>> -	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
>>> +	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0)
>>> +	  | stack_mem_tagging_prot ());
>>>   }
>> I think we should clean this up and arrange for the dynamic linker to
>> determine the PROT_* flags for stacks created at run time, so that we
>> can use that variable directly without further processing.

> IMHO, we cannot escape from abstracting somehow the final flags as
> above with PF_X.  The problem being that each OS would have each own
> flags and syscalls and dynamic linker is OS agnostic, AFAIK.

The dynamic linker has both Linux- and architecture-specific code.
I would like to see

  GLRO (dl_stack_prot_flags)

instead of:

  stack_prot ()

Then we can remove the GL(dl_stack_flags) variable.

Thanks,
Florian
diff mbox series

Patch

diff --git a/elf/dl-mte-stack.c b/elf/dl-mte-stack.c
index f0063b54a0..f5b8293c7a 100644
--- a/elf/dl-mte-stack.c
+++ b/elf/dl-mte-stack.c
@@ -25,3 +25,4 @@  _dl_mte_stack_protect (void *stack_end)
   return ENOSYS;
 }
 rtld_hidden_def (_dl_mte_stack_protect)
+
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 800ca89720..6155157063 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -153,7 +153,8 @@  static int allocate_stack_mode = ALLOCATE_GUARD_MADV_GUARD;
 static inline int stack_prot (void)
 {
   return (PROT_READ | PROT_WRITE
-	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
+	  | ((GL(dl_stack_flags) & PF_X) ? PROT_EXEC : 0)
+	  | stack_mem_tagging_prot ());
 }
 
 static void *
diff --git a/nptl/nptl-stack.c b/nptl/nptl-stack.c
index c049c5133c..34fbba1cd1 100644
--- a/nptl/nptl-stack.c
+++ b/nptl/nptl-stack.c
@@ -145,3 +145,10 @@  __pthread_get_minstack (const pthread_attr_t *attr)
 	  + PTHREAD_STACK_MIN);
 }
 libc_hidden_def (__pthread_get_minstack)
+
+int
+__stack_mem_tagging_prot (void)
+{
+  return 0;
+}
+weak_alias (__stack_mem_tagging_prot, stack_mem_tagging_prot)
diff --git a/nptl/nptl-stack.h b/nptl/nptl-stack.h
index ac672e16cf..2bfcaf7252 100644
--- a/nptl/nptl-stack.h
+++ b/nptl/nptl-stack.h
@@ -61,4 +61,6 @@  __nptl_tls_static_size_for_stack (void)
   return roundup (GLRO (dl_tls_static_size), GLRO (dl_tls_static_align));
 }
 
+int stack_mem_tagging_prot (void);
+
 #endif /* _NPTL_STACK_H */
diff --git a/sysdeps/unix/sysv/linux/aarch64/Makefile b/sysdeps/unix/sysv/linux/aarch64/Makefile
index 8fb0e65b2a..e91a756836 100644
--- a/sysdeps/unix/sysv/linux/aarch64/Makefile
+++ b/sysdeps/unix/sysv/linux/aarch64/Makefile
@@ -10,6 +10,11 @@  sysdep-dl-routines += \
   dl-mte-stack
 endif
 
+ifeq ($(subdir),nptl)
+routines += \
+  dl-mte-stack
+endif
+
 ifeq ($(subdir),stdlib)
 gen-as-const-headers += ucontext_i.sym
 endif
diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c b/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c
index a4bc82c99b..7ed92855ae 100644
--- a/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c
+++ b/sysdeps/unix/sysv/linux/aarch64/dl-mte-stack.c
@@ -42,7 +42,7 @@  void
 _dl_mte_stack_protect (void *stack_end)
 {
   if ((GLRO(dl_aarch64_cpu_features).mte_state & AARCH64_CPU_FEATURE_MTE_STATE_STACK) == 0
-      ||  !_dl_mte_mode ())
+      || !_dl_mte_mode ())
     return;
 
   int errval = 0;
@@ -72,3 +72,12 @@  cannot set stack with PROT_MTE");
     }
 }
 
+int
+stack_mem_tagging_prot (void)
+{
+  if ((GLRO(dl_aarch64_cpu_features).mte_state & AARCH64_CPU_FEATURE_MTE_STATE_STACK) == 8
+      && _dl_mte_mode ())
+    return PROT_MTE;
+  else
+    return 0;
+}