| Message ID | 20250114160328.2031684-14-yury.khrustalev@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Add support for Guarded Control Stack extension | expand |
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) > > Describe new tunable name in the manual. > --- > manual/tunables.texi | 32 +++++++++++++++++++ > sysdeps/aarch64/dl-tunables.list | 5 +++ > .../unix/sysv/linux/aarch64/cpu-features.c | 9 ++++-- > sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c | 13 ++++++++ > 4 files changed, 57 insertions(+), 2 deletions(-) > > diff --git a/manual/tunables.texi b/manual/tunables.texi > index 118f490010..8f989efbcd 100644 > --- a/manual/tunables.texi > +++ b/manual/tunables.texi > @@ -678,6 +678,38 @@ disabled by default. > This tunable is specific to AArch64. > @end deftp > > +@deftp Tunable glibc.cpu.aarch64_gcs_policy > +The GCS policy tunable defines how the GCS tunable and the GCS marking > +of binaries being loaded turn into GCS state (enabled, disabled or error). > + > +If @code{glibc.cpu.aarch64_gcs} is not set, the GCS policy tunable has no > +effect. > + > +GCS policy @code{0} means that GCS is enabled if @code{glibc.cpu.aarch64_gcs} > +is set. Any GCS marking mismatch is ignored. > + > +GCS policy @code{1} means that GCS is enabled if @code{glibc.cpu.aarch64_gcs} > +is set and all binaries are GCS-marked. If GCS is required and an incompatible > +library is loaded via @code{dlopen}, it is an error, otherwise any > +incompatible binary will disable GCS. > + > +GCS policy @code{2} means that GCS is enabled if @code{glibc.cpu.aarch64_gcs} > +is set and all binaries are GCS-marked. Any incompatible binary will result in > +an error. > + > +For the avoidance of doubt, if @code{glibc.cpu.aarch64_gcs} is set and GCS > +marking is not ignored (i.e. @code{glibc.cpu.aarch64_gcs_policy} is set to > +either @code{1} or @code{2}), then loading an incompatible library via > +@code{dlopen} is always an error, otherwise, with policy @code{1} GCS will be > +disabled when one of the loaded binaries is not GCS-marked and with policy > +@code{2} it will be an error, so GCS policy @code{2} enforces all loaded > +binaries to be GCS-marked. 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 default is @code{0} . > + > +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. */
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. > > > + > > +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. */ >
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. */ >>
On Thu, Jan 16, 2025 at 09:37:37AM -0300, Adhemerval Zanella Netto wrote: > >>> > >>> 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 I'm not sure if this is really a security issue as the policy tunable is controlled via environment variable, so if policy could be somehow set to 1 by an attacker, then this attacker would likely also be able to set the glibc.cpu.aarch64_gcs tunable to 0. > 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) This means that, when glibc.cpu.aarch64_gcs is set, with policy 1 we always fail if there is marking mismatch **regardless** of loading being via dlopen() or not. > 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. We already error on dlopen of unmarked library when glibc.cpu.aarch64_gcs is set with the current implementation for any non-zero policy. Thanks, Yury
diff --git a/manual/tunables.texi b/manual/tunables.texi index 118f490010..8f989efbcd 100644 --- a/manual/tunables.texi +++ b/manual/tunables.texi @@ -678,6 +678,38 @@ disabled by default. This tunable is specific to AArch64. @end deftp +@deftp Tunable glibc.cpu.aarch64_gcs_policy +The GCS policy tunable defines how the GCS tunable and the GCS marking +of binaries being loaded turn into GCS state (enabled, disabled or error). + +If @code{glibc.cpu.aarch64_gcs} is not set, the GCS policy tunable has no +effect. + +GCS policy @code{0} means that GCS is enabled if @code{glibc.cpu.aarch64_gcs} +is set. Any GCS marking mismatch is ignored. + +GCS policy @code{1} means that GCS is enabled if @code{glibc.cpu.aarch64_gcs} +is set and all binaries are GCS-marked. If GCS is required and an incompatible +library is loaded via @code{dlopen}, it is an error, otherwise any +incompatible binary will disable GCS. + +GCS policy @code{2} means that GCS is enabled if @code{glibc.cpu.aarch64_gcs} +is set and all binaries are GCS-marked. Any incompatible binary will result in +an error. + +For the avoidance of doubt, if @code{glibc.cpu.aarch64_gcs} is set and GCS +marking is not ignored (i.e. @code{glibc.cpu.aarch64_gcs_policy} is set to +either @code{1} or @code{2}), then loading an incompatible library via +@code{dlopen} is always an error, otherwise, with policy @code{1} GCS will be +disabled when one of the loaded binaries is not GCS-marked and with policy +@code{2} it will be an error, so GCS policy @code{2} enforces all loaded +binaries to be GCS-marked. + +The default is @code{0} . + +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. */
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) Describe new tunable name in the manual. --- manual/tunables.texi | 32 +++++++++++++++++++ sysdeps/aarch64/dl-tunables.list | 5 +++ .../unix/sysv/linux/aarch64/cpu-features.c | 9 ++++-- sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c | 13 ++++++++ 4 files changed, 57 insertions(+), 2 deletions(-)