[PATCH v8 13/20] aarch64: Add glibc.cpu.aarch64_gcs_policy tunable

[Adhemerval Zanella Netto]

On 16/01/25 07:46, Yury Khrustalev wrote:
> On Wed, Jan 15, 2025 at 01:46:03PM -0300, Adhemerval Zanella Netto wrote:
>>
>> On 14/01/25 13:03, Yury Khrustalev wrote:
>>> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
>>>
>>> Policy sets how GCS tunable and GCS marking turns into gcs state:
>>>
>>> 0: state = tunable
>>> 1: state = marking ? tunable : (tunable && dlopen ? err : 0)
>>> 2: state = marking ? tunable : (tunable ? err : 0)
>>>
>>
>> Is the policy '1' really a good semantic? I know that for testing and transition to
>> GCS enablement it might make sense, but we recently removed the non-executable stack
>> switch for dynamic shared object through dlopen (0ca8785a28515291d4ef074b5b6cfb27434c1d2b)
>> because this was used as vector for a RCE.
>>
>> I know that is gated through a environment variable, but even though it opens some
>> possibility to silent disable GCS in a misconfigured environment.
> 
> The value 1 is a lenient form of policy to allow applocations that fully
> support GCS to benefit from it while the apps that have dependencies that
> don't yet support GCS would continue to work in the same environment. This
> is indeed to help with transition, and we think this approach is reasonable.

This is a potentially security issue and I don't think we should allow it
Similar to how now we enforce non-executable stacks, I think we should 
enforce on program loading, or return an error for dlopen.  Meaning just:

  0: state = tunable
  1: state = marking ? tunable : (tunable ? err : 0)

It means dlopen will fail if the object does not contain the GCS marking
if GCS is enabled; which I think is the expected behavior for most deployments.

> 
>>
>>> +
>>> +The default is @code{0} .
> 
> I'll fix this typo in the next version of the patch.
> 
>>> +
>>> +This tunable is specific to AArch64.
>>> +@end deftp
>>> +
>>>  @node Memory Related Tunables
>>>  @section Memory Related Tunables
>>>  @cindex memory related tunables
>>> diff --git a/sysdeps/aarch64/dl-tunables.list b/sysdeps/aarch64/dl-tunables.list
>>> index 4b28341b72..7fbd77a41b 100644
>>> --- a/sysdeps/aarch64/dl-tunables.list
>>> +++ b/sysdeps/aarch64/dl-tunables.list
>>> @@ -26,5 +26,10 @@ glibc {
>>>        minval: 0
>>>        default: 0
>>>      }
>>> +    aarch64_gcs_policy {
>>> +      type: UINT_64
>>> +      minval: 0
>>> +      default: 0
>>> +    }
>>>    }
>>>  }
>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>>> index 1ecf6cd176..5d03a4b01b 100644
>>> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>>> @@ -182,6 +182,11 @@ init_cpu_features (struct cpu_features *cpu_features)
>>>  #endif
>>>  
>>>    if (GLRO (dl_hwcap) & HWCAP_GCS)
>>> -    /* GCS status may be updated later by binary compatibility checks.  */
>>> -    GL (dl_aarch64_gcs) = TUNABLE_GET (glibc, cpu, aarch64_gcs, uint64_t, 0);
>>> +    {
>>> +      /* GCS status may be updated later by binary compatibility checks.  */
>>> +      GL (dl_aarch64_gcs) = TUNABLE_GET (glibc, cpu, aarch64_gcs, uint64_t, 0);
>>> +      /* Fixed GCS policy.  */
>>> +      GLRO (dl_aarch64_gcs_policy) =
>>> +	TUNABLE_GET (glibc, cpu, aarch64_gcs_policy, uint64_t, 0);
>>> +    }
>>>  }
>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
>>> index 66287b4216..bcfc3fe030 100644
>>> --- a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
>>> @@ -54,6 +54,19 @@ PROCINFO_CLASS struct cpu_features _dl_aarch64_cpu_features
>>>  # else
>>>  ,
>>>  # endif
>>> +# if !defined PROCINFO_DECL && defined SHARED
>>> +  ._dl_aarch64_gcs_policy
>>> +# else
>>> +PROCINFO_CLASS uint64_t _dl_aarch64_gcs_policy
>>> +# endif
>>> +# ifndef PROCINFO_DECL
>>> += 0
>>> +# endif
>>> +# if !defined SHARED || defined PROCINFO_DECL
>>> +;
>>> +# else
>>> +,
>>> +# endif
>>>  #endif
>>>  
>>>  /* Number of HWCAP bits set.  */
>>