From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Szabolcs Nagy <szabolcs.nagy@arm.com>
Cc: libc-alpha@sourceware.org, jma14 <jma14@rice.edu>,
John Mellor-Crummey <johnmc@rice.edu>
Subject: Re: [PATCH v9 4/4] elf: Fix runtime linker auditing on aarch64 (BZ #26643)
Date: Tue, 11 Jan 2022 13:49:33 -0300 [thread overview]
Message-ID: <5de33ad5-d86a-3370-4a24-fbb3d6a4ac42@linaro.org> (raw)
In-Reply-To: <20220111111645.GX3294453@arm.com>
On 11/01/2022 08:16, Szabolcs Nagy wrote:
> The 01/03/2022 10:25, Adhemerval Zanella via Libc-alpha wrote:
>> From: Ben Woodard <woodard@redhat.com>
>>
>> The rtld audit support show two problems on aarch64:
>>
>> 1. _dl_runtime_resolve does not preserve x8, the indirect result
>> location register, which might generate wrong result calls
>> depending of the function signature.
>>
>> 2. The NEON Q registers pushed onto the stack by _dl_runtime_resolve
>> were twice the size of D registers extracted from the stack frame by
>> _dl_runtime_profile.
>>
>> While 2. might result in wrong information passed on the PLT tracing,
>> 1. generates wrong runtime behaviour.
>>
>> The aarch64 rtld audit support is change to:
>>
>> * Both La_aarch64_regs and La_aarch64_retval are expanded to include
>> both x8 and the full sized NEON V registers, as defined by the
>> ABI.
>>
>> * dl_runtime_profile needed to extract registers saved by
>> _dl_runtime_resolve and put them into the new correctly sized
>> La_aarch64_regs structure.
>>
>> * The LAV_CURRENT check is change to only accept new audit modules
>> to avoid the undefined behavior of not save/restore x8.
>>
>> * Different than other architectures, audit modules older than
>> LAV_CURRENT are rejected (both La_aarch64_regs and La_aarch64_retval
>> changes layout and it does not work the complexity to support
>> multiple audit interfaces).
>>
>
> i'd mention here that a field is reserved for extension
> so variant pcs symbols can be supported to with plt audit.
>
Ack, I changed to:
* Different than other architectures, audit modules older than
LAV_CURRENT are rejected (both La_aarch64_regs and La_aarch64_retval
changed their layout and the it does worth the to support multiple
audit interface with the inherent aarch64 issues).
* A new field is also reserved on both La_aarch64_regs and
La_aarch64_retval to support variant pcs symbols.
>> Similar to x86, a new La_aarch64_vector type to represent the NEON
>> register is added on the La_aarch64_regs (so each type can be accessed
>> directly).
>>
>> Since LAV_CURRENT was already bumped to support bind-now, there is
>> no need to increase it again.
>>
>> Checked on aarch64-linux-gnu.
>>
>> Co-authored-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>> ---
>> NEWS | 4 +
>> elf/rtld.c | 3 +-
>> sysdeps/aarch64/Makefile | 20 ++++
>> sysdeps/aarch64/bits/link.h | 26 +++--
>> sysdeps/aarch64/dl-audit-check.h | 28 +++++
>> sysdeps/aarch64/dl-link.sym | 6 +-
>> sysdeps/aarch64/dl-trampoline.S | 97 +++++++++++------
>> sysdeps/aarch64/tst-audit26.c | 37 +++++++
>> sysdeps/aarch64/tst-audit26mod.c | 33 ++++++
>> sysdeps/aarch64/tst-audit26mod.h | 50 +++++++++
>> sysdeps/aarch64/tst-audit27.c | 64 +++++++++++
>> sysdeps/aarch64/tst-audit27mod.c | 95 ++++++++++++++++
>> sysdeps/aarch64/tst-audit27mod.h | 67 ++++++++++++
>> sysdeps/aarch64/tst-auditmod26.c | 103 ++++++++++++++++++
>> sysdeps/aarch64/tst-auditmod27.c | 180 +++++++++++++++++++++++++++++++
>> sysdeps/generic/dl-audit-check.h | 23 ++++
>> 16 files changed, 789 insertions(+), 47 deletions(-)
>> create mode 100644 sysdeps/aarch64/dl-audit-check.h
>> create mode 100644 sysdeps/aarch64/tst-audit26.c
>> create mode 100644 sysdeps/aarch64/tst-audit26mod.c
>> create mode 100644 sysdeps/aarch64/tst-audit26mod.h
>> create mode 100644 sysdeps/aarch64/tst-audit27.c
>> create mode 100644 sysdeps/aarch64/tst-audit27mod.c
>> create mode 100644 sysdeps/aarch64/tst-audit27mod.h
>> create mode 100644 sysdeps/aarch64/tst-auditmod26.c
>> create mode 100644 sysdeps/aarch64/tst-auditmod27.c
>> create mode 100644 sysdeps/generic/dl-audit-check.h
>>
>> diff --git a/NEWS b/NEWS
>> index b2999e4881..b0272ae464 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -130,6 +130,10 @@ Deprecated and removed features, and other changes affecting compatibility:
>> proper bind-now support. The loader now advertises on the la_symbind
>> flags that PLT trace is not possible.
>>
>> +* The audit interface on aarch64 is extended to support both the indirect
>> + result location register (x8) and NEON Q register. This makes old audit
>> + modules to be rejected by the loader.
>> +
>
> i would say that 'Old audit modules are rejected by the loader.'
> (without "this makes..")
Ack.
>
>> diff --git a/sysdeps/aarch64/bits/link.h b/sysdeps/aarch64/bits/link.h
>> index e64f36d3f3..2479abc4fb 100644
>> --- a/sysdeps/aarch64/bits/link.h
>> +++ b/sysdeps/aarch64/bits/link.h
>> @@ -20,23 +20,31 @@
>> # error "Never include <bits/link.h> directly; use <link.h> instead."
>> #endif
>>
>> +typedef union
>> +{
>> + float s;
>> + double d;
>> + long double q;
>> +} La_aarch64_vector;
>> +
>> /* Registers for entry into PLT on AArch64. */
>> typedef struct La_aarch64_regs
>> {
>> - uint64_t lr_xreg[8];
>> - uint64_t lr_dreg[8];
>> - uint64_t lr_sp;
>> - uint64_t lr_lr;
>> + uint64_t lr_xreg[9];
>> + La_aarch64_vector lr_vreg[8];
>> + uint64_t lr_sp;
>> + uint64_t lr_lr;
>> + void *lr_vpcs;
>> } La_aarch64_regs;
>>
>> /* Return values for calls from PLT on AArch64. */
>> typedef struct La_aarch64_retval
>> {
>> - /* Up to two integer registers can be used for a return value. */
>> - uint64_t lrv_xreg[2];
>> - /* Up to four D registers can be used for a return value. */
>> - uint64_t lrv_dreg[4];
>> -
>> + /* Up to eight integer registers can be used for a return value. */
>> + uint64_t lrv_xreg[8];
>> + /* Up to eight V registers can be used for a return value. */
>> + La_aarch64_vector lrv_vreg[8];
>> + void *lrv_vpcs;
>> } La_aarch64_retval;
>> __BEGIN_DECLS
>>
>
> this looks ok.
>
>> diff --git a/sysdeps/aarch64/dl-audit-check.h b/sysdeps/aarch64/dl-audit-check.h
>> new file mode 100644
>> index 0000000000..0efb5de6b3
>> --- /dev/null
>> +++ b/sysdeps/aarch64/dl-audit-check.h
>> @@ -0,0 +1,28 @@
>> +/* rtld-audit version check. AArch64 version.
>> + Copyright (C) 2021 Free Software Foundation, Inc.
>
> the year will have to be updated.
>
> same for other new files.
Ack.
>
>> diff --git a/sysdeps/aarch64/dl-trampoline.S b/sysdeps/aarch64/dl-trampoline.S
>> index a403863ef9..692611341d 100644
>> --- a/sysdeps/aarch64/dl-trampoline.S
>> +++ b/sysdeps/aarch64/dl-trampoline.S
>> @@ -45,7 +45,8 @@ _dl_runtime_resolve:
>>
>> cfi_rel_offset (lr, 8)
>>
>> - /* Save arguments. */
>> + /* Note: Saving x9 is not required by the ABI but the assember requires
>> + the immediate values of operand 3 to be a multiple of 16 */
>> stp x8, x9, [sp, #-(80+8*16)]!
>> cfi_adjust_cfa_offset (80+8*16)
>> cfi_rel_offset (x8, 0)
>> @@ -142,13 +143,17 @@ _dl_runtime_profile:
>> Stack frame layout:
>> [sp, #...] lr
>> [sp, #...] &PLTGOT[n]
>> - [sp, #96] La_aarch64_regs
>> - [sp, #48] La_aarch64_retval
>> - [sp, #40] frame size return from pltenter
>> - [sp, #32] dl_profile_call saved x1
>> - [sp, #24] dl_profile_call saved x0
>> - [sp, #16] t1
>> - [sp, #0] x29, lr <- x29
>> + -----------------------
>> + [sp, #384] La_aarch64_regs::lr_xreg (x0-x8)
>> + [sp, #256] La_aarch64_regs::lr_vreg (q0-q7)
>> + [sp, #240] La_aarch64_regs::sp and La_aarch64_regs::lr
>> + [sp, #176] La_aarch64_retval::lrv_xreg (x0-x7)
>> + [sp, # 48] La_aarch64_retval::lrv_vreg (q0-q7)
>> + [sp, # 40] frame size return from pltenter
>> + [sp, # 32] dl_profile_call saved x1
>> + [sp, # 24] dl_profile_call saved x0
>> + [sp, # 16] t1
>> + [sp, # 0] x29, lr <- x29
>> */
>
> the layout in the comment looks backwards.
It follows the convention of the about layout:
/* AArch64 we get called with:
ip0 &PLTGOT[2]
ip1 temp(dl resolver entry point)
[sp, #8] lr
[sp, #0] &PLTGOT[n]
[...]
>
> the tests look good.
> thanks.
next prev parent reply other threads:[~2022-01-11 16:49 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-03 13:25 [PATCH v9 0/4] Multiple rtld-audit fixes Adhemerval Zanella
2022-01-03 13:25 ` [PATCH v9 1/4] elf: Add la_activity during application exit Adhemerval Zanella
2022-01-04 17:13 ` Jonathon Anderson
2022-01-03 13:25 ` [PATCH v9 2/4] elf: Fix initial-exec TLS access on audit modules (BZ #28096) Adhemerval Zanella
2022-01-03 13:25 ` [PATCH v9 3/4] elf: Issue la_symbind for bind-now (BZ #23734) Adhemerval Zanella
2022-01-03 13:25 ` [PATCH v9 4/4] elf: Fix runtime linker auditing on aarch64 (BZ #26643) Adhemerval Zanella
2022-01-11 11:16 ` Szabolcs Nagy
2022-01-11 16:49 ` Adhemerval Zanella [this message]
2022-01-11 17:09 ` Szabolcs Nagy
2022-01-11 18:12 ` Adhemerval Zanella
2022-01-13 16:24 ` Szabolcs Nagy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5de33ad5-d86a-3370-4a24-fbb3d6a4ac42@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=jma14@rice.edu \
--cc=johnmc@rice.edu \
--cc=libc-alpha@sourceware.org \
--cc=szabolcs.nagy@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).