[v4,19/22] aarch64: Process gnu properties in static exe
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Test passed
|
Commit Message
From: Szabolcs Nagy <szabolcs.nagy@arm.com>
Unlike for BTI, the kernel does not process GCS properties so update
GL(dl_aarch64_gcs) before the GCS status is set.
---
sysdeps/unix/sysv/linux/aarch64/libc-start.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
Comments
On 29/11/24 13:37, Yury Khrustalev wrote:
> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
>
> Unlike for BTI, the kernel does not process GCS properties so update
> GL(dl_aarch64_gcs) before the GCS status is set.
Although it would only used for aarch64, I think we should make this change
generic as I suggested for memory sealing support [1].
[1] https://patchwork.sourceware.org/project/glibc/patch/20240930200831.1669010-3-adhemerval.zanella@linaro.org/
> ---
> sysdeps/unix/sysv/linux/aarch64/libc-start.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/aarch64/libc-start.h b/sysdeps/unix/sysv/linux/aarch64/libc-start.h
> index 0dd102c374..1cc4dab818 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/libc-start.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/libc-start.h
> @@ -21,6 +21,8 @@
>
> #ifndef SHARED
>
> +# include <dl-prop.h>
> +
> # ifndef PR_SET_SHADOW_STACK_STATUS
> # define PR_GET_SHADOW_STACK_STATUS 74
> # define PR_SET_SHADOW_STACK_STATUS 75
> @@ -36,6 +38,16 @@ aarch64_libc_setup_tls (void)
> {
> __libc_setup_tls ();
>
> + struct link_map *main_map = _dl_get_dl_main_map ();
> + const ElfW(Phdr) *phdr = GL(dl_phdr);
> + const ElfW(Phdr) *ph;
> + for (ph = phdr; ph < phdr + GL(dl_phnum); ph++)
> + if (ph->p_type == PT_GNU_PROPERTY)
> + {
> + _dl_process_pt_gnu_property (main_map, -1, ph);
> + _rtld_main_check (main_map, _dl_argv[0]);
> + break;
> + }
> uint64_t s = GL(dl_aarch64_gcs);
> if (s != 0)
> INLINE_SYSCALL_CALL (prctl, PR_SET_SHADOW_STACK_STATUS, s, 0, 0, 0);
On Tue, Dec 03, 2024 at 03:56:45PM -0300, Adhemerval Zanella Netto wrote:
>
>
> On 29/11/24 13:37, Yury Khrustalev wrote:
> > From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> >
> > Unlike for BTI, the kernel does not process GCS properties so update
> > GL(dl_aarch64_gcs) before the GCS status is set.
>
> Although it would only used for aarch64, I think we should make this change
> generic as I suggested for memory sealing support [1].
>
> [1] https://patchwork.sourceware.org/project/glibc/patch/20240930200831.1669010-3-adhemerval.zanella@linaro.org/
I can see some similatiry however in the case of GCS it's crucial to initialise
it as early as possible. In the patch [1] you referenced this setup would occur
after a number of function calls (as far as I can tell from the sources).
I agree that it makes sense to make this change generic eventually, but at this
point I don't see how to do it. I'd like to leave it AArch64 specific for now.
Thanks,
Yury
On 06/12/24 08:55, Yury Khrustalev wrote:
> On Tue, Dec 03, 2024 at 03:56:45PM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>> On 29/11/24 13:37, Yury Khrustalev wrote:
>>> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
>>>
>>> Unlike for BTI, the kernel does not process GCS properties so update
>>> GL(dl_aarch64_gcs) before the GCS status is set.
>>
>> Although it would only used for aarch64, I think we should make this change
>> generic as I suggested for memory sealing support [1].
>>
>> [1] https://patchwork.sourceware.org/project/glibc/patch/20240930200831.1669010-3-adhemerval.zanella@linaro.org/
>
> I can see some similatiry however in the case of GCS it's crucial to initialise
> it as early as possible. In the patch [1] you referenced this setup would occur
> after a number of function calls (as far as I can tell from the sources).
>
> I agree that it makes sense to make this change generic eventually, but at this
> point I don't see how to do it. I'd like to leave it AArch64 specific for now.
The main problem is to enable memory sealing would incur in double in processing
PT_GNU_PROPERTY multiple time, which its wasteful.
Maybe we can add another call to process the PT_NOTE / PT_GNU_PROPERTY prior
ARCH_SETUP_TLS (since notes/property processing should not rely on TCB
support).
Hello Adhemerval
On Fri, Dec 06, 2024 at 11:05:59AM -0300, Adhemerval Zanella Netto wrote:
>
>
> On 06/12/24 08:55, Yury Khrustalev wrote:
> > On Tue, Dec 03, 2024 at 03:56:45PM -0300, Adhemerval Zanella Netto wrote:
> >>
> >>
> >> On 29/11/24 13:37, Yury Khrustalev wrote:
> >>> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> >>>
> >>> Unlike for BTI, the kernel does not process GCS properties so update
> >>> GL(dl_aarch64_gcs) before the GCS status is set.
> >>
> >> Although it would only used for aarch64, I think we should make this change
> >> generic as I suggested for memory sealing support [1].
> >>
> >> [1] https://patchwork.sourceware.org/project/glibc/patch/20240930200831.1669010-3-adhemerval.zanella@linaro.org/
> >
> > I can see some similatiry however in the case of GCS it's crucial to initialise
> > it as early as possible. In the patch [1] you referenced this setup would occur
> > after a number of function calls (as far as I can tell from the sources).
> >
> > I agree that it makes sense to make this change generic eventually, but at this
> > point I don't see how to do it. I'd like to leave it AArch64 specific for now.
>
> The main problem is to enable memory sealing would incur in double in processing
> PT_GNU_PROPERTY multiple time, which its wasteful.
This is a good point.
> Maybe we can add another call to process the PT_NOTE / PT_GNU_PROPERTY prior
> ARCH_SETUP_TLS (since notes/property processing should not rely on TCB
> support).
I have a patch for this ready and tested that replaces the one in this thread.
It's not part of v5 [1] I've submitted last Friday. As soon as other patches in
this series have been reviewed, I'll resend change to address your comments above.
I hope this helps and won't delay merge of the whole seriese before the deadline
for the upcoming release.
Kind regards,
Yury
Hello Adhemerval
On Tue, Dec 10, 2024 at 09:26:47AM +0000, Yury Khrustalev wrote:
> Hello Adhemerval
>
> On Fri, Dec 06, 2024 at 11:05:59AM -0300, Adhemerval Zanella Netto wrote:
> >
> >
> > On 06/12/24 08:55, Yury Khrustalev wrote:
> > > On Tue, Dec 03, 2024 at 03:56:45PM -0300, Adhemerval Zanella Netto wrote:
> > >>
> > >>
> > >> On 29/11/24 13:37, Yury Khrustalev wrote:
> > >>> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> > >>>
> > >>> Unlike for BTI, the kernel does not process GCS properties so update
> > >>> GL(dl_aarch64_gcs) before the GCS status is set.
> > >>
> > >> Although it would only used for aarch64, I think we should make this change
> > >> generic as I suggested for memory sealing support [1].
> > >>
> > >> [1] https://patchwork.sourceware.org/project/glibc/patch/20240930200831.1669010-3-adhemerval.zanella@linaro.org/
> > >
> > > I can see some similatiry however in the case of GCS it's crucial to initialise
> > > it as early as possible. In the patch [1] you referenced this setup would occur
> > > after a number of function calls (as far as I can tell from the sources).
> > >
> > > I agree that it makes sense to make this change generic eventually, but at this
> > > point I don't see how to do it. I'd like to leave it AArch64 specific for now.
> >
> > The main problem is to enable memory sealing would incur in double in processing
> > PT_GNU_PROPERTY multiple time, which its wasteful.
>
> This is a good point.
>
> > Maybe we can add another call to process the PT_NOTE / PT_GNU_PROPERTY prior
> > ARCH_SETUP_TLS (since notes/property processing should not rely on TCB
> > support).
>
> I have a patch for this ready and tested that replaces the one in this thread.
> It's not part of v5 [1] I've submitted last Friday. As soon as other patches in
> this series have been reviewed, I'll resend change to address your comments above.
> I hope this helps and won't delay merge of the whole seriese before the deadline
> for the upcoming release.
I've sent a patch that introduces generic function for processing PT_NOTE / PT_GNU_PROPERTY.
It is part of the v6 of the GCS series [1].
I would appreciate if you could review this as I hope to have this whole serience accepted
before the holidays.
Thanks,
Yury
[1] https://inbox.sourceware.org/libc-alpha/20241212143749.767747-1-yury.khrustalev@arm.com/
@@ -21,6 +21,8 @@
#ifndef SHARED
+# include <dl-prop.h>
+
# ifndef PR_SET_SHADOW_STACK_STATUS
# define PR_GET_SHADOW_STACK_STATUS 74
# define PR_SET_SHADOW_STACK_STATUS 75
@@ -36,6 +38,16 @@ aarch64_libc_setup_tls (void)
{
__libc_setup_tls ();
+ struct link_map *main_map = _dl_get_dl_main_map ();
+ const ElfW(Phdr) *phdr = GL(dl_phdr);
+ const ElfW(Phdr) *ph;
+ for (ph = phdr; ph < phdr + GL(dl_phnum); ph++)
+ if (ph->p_type == PT_GNU_PROPERTY)
+ {
+ _dl_process_pt_gnu_property (main_map, -1, ph);
+ _rtld_main_check (main_map, _dl_argv[0]);
+ break;
+ }
uint64_t s = GL(dl_aarch64_gcs);
if (s != 0)
INLINE_SYSCALL_CALL (prctl, PR_SET_SHADOW_STACK_STATUS, s, 0, 0, 0);