Sorry for the slow review.
Matthieu Longo <matthieu.lo...@arm.com> writes:
This patch is only a refactoring of the existing implementation
of PAuth and returned-address signing. The existing behavior is
preserved.
_Unwind_FrameState already contains several CIE and FDE information
(see the attributes below the comment "The information we care
about from the CIE/FDE" in libgcc/unwind-dw2.h).
The patch aims at moving the information from DWARF CIE (signing
key stored in the augmentation string) and FDE (the used signing
method) into _Unwind_FrameState along the already-stored CIE and
FDE information.
Note: those information have to be saved in frame_state_reg_info
instead of _Unwind_FrameState as they need to be savable by
DW_CFA_remember_state and restorable by DW_CFA_restore_state, that
both rely on the attribute "prev".
Those new information in _Unwind_FrameState simplifies the look-up
of the signing key when the return address is demangled. It also
allows future signing methods to be easily added.
_Unwind_FrameState is not a part of the public API of libunwind,
so the change is backward compatible.
A new architecture-specific handler MD_ARCH_EXTENSION_FRAME_INIT
allows to reset values (if needed) in the frame state and unwind
context before changing the frame state to the caller context.
A new architecture-specific handler MD_ARCH_EXTENSION_CIE_AUG_HANDLER
isolates the architecture-specific augmentation strings in AArch64
backend, and allows others architectures to reuse augmentation
strings that would have clashed with AArch64 DWARF extensions.
aarch64_demangle_return_addr, DW_CFA_AARCH64_negate_ra_state and
DW_CFA_val_expression cases in libgcc/unwind-dw2-execute_cfa.h
were documented to clarify where the value of the RA state register
is stored (FS and CONTEXT respectively).
The abstraction generally looks good. My main comment is that,
if we are putting the new behaviour behind target macros (which I
agree is a good idea), could we also have a target macro to abstract:
+#if defined(__aarch64__) && !defined (__ILP32__)
+ unsigned char signing_key;
+#endif
? E.g. something like:
#ifdef MD_CFI_STATE
MD_CFI_STATE md;
#endif
with aarch64 defining:
#define MD_CFI_STATE struct { unsigned char signing_key; }
(Names and organisation are just suggestions.)
It might be good to try running the patch through
contrig/check_GNU_style.py
since the GCC coding standards are quite picky about formatting and
stylistic issues. The main ones I spotted were:
- In files that already use block comments, all comments should be
block comments rather than // comments. The comments should end
with ". " (full stop and two spaces).
- Block comments are formatted as:
/* Line 1
Line 2. */
rather than as:
/* Line 1
* Line 2. */
- Function names are generally all lowrecase (e.g. "ra" rather than "RA").
- In function calls, there should be a space between the function and
the opening "("
- For pointer types, there should be a space before a "*" (or string of
"*"s), but no space afterwards.
- "const" qualifiers generally go before the type that they qualify,
rather than afterwards.
- The line width should be 80 characters or fewer. (The patch was pretty
good about this, but there were a couple of long lines).
- In multi-line conditions, the ||s and &&s go at the beginning of lines,
rather than at the end. (Same for infix operators in general.)
More detailed comments below.
libgcc/ChangeLog:
* config/aarch64/aarch64-unwind.h
(AARCH64_DWARF_RA_STATE_MASK): The mask for RA state register.
(aarch64_RA_signing_method_t): The diversifiers used to sign a
function's return address.
(aarch64_pointer_auth_key): The key used to sign a function's
return address.
(aarch64_cie_signed_with_b_key): Deleted as the signing key is
available now in _Unwind_FrameState.
(MD_ARCH_EXTENSION_CIE_AUG_HANDLER): New CIE augmentation string
handler for architecture extensions.
(MD_ARCH_EXTENSION_FRAME_INIT): New architecture-extension
initialization routine for DWARF frame state and context before
execution of DWARF instructions.
(aarch64_context_RA_state_get): Read RA state register from CONTEXT.
(aarch64_RA_state_get): Read RA state register from FS.
(aarch64_RA_state_set): Write RA state register into FS.
(aarch64_RA_state_toggle): Toggle RA state register in FS.
(aarch64_cie_aug_handler): Handler AArch64 augmentation strings.
(aarch64_arch_extension_frame_init): Initialize defaults for the
signing key (PAUTH_KEY_A), and RA state register (RA_no_signing).
(aarch64_demangle_return_addr): Rely on the frame registers and
the signing_key attribute in _Unwind_FrameState.
* unwind-dw2-execute_cfa.h:
Use the right alias DW_CFA_AARCH64_negate_ra_state for __aarch64__
instead of DW_CFA_GNU_window_save.
(DW_CFA_AARCH64_negate_ra_state): Save the signing method in RA
state register. Toggle RA state register without resetting 'how'
to REG_UNSAVED.
* unwind-dw2.c:
(extract_cie_info): Save the signing key in the current
_Unwind_FrameState while parsing the augmentation data.
(uw_frame_state_for): Reset some attributes related to architecture
extensions in _Unwind_FrameState.
(uw_update_context): Move authentication code to AArch64 unwinding.
* unwind-dw2.h (enum register_rule): Give a name to the existing
enum for the register rules, and replace 'unsigned char' by 'enum
register_rule' to facilitate debugging in GDB.
(_Unwind_FrameState): Add a new architecture-extension attribute
to store the signing key.
---
libgcc/config/aarch64/aarch64-unwind.h | 154 ++++++++++++++++++++-----
libgcc/unwind-dw2-execute_cfa.h | 34 ++++--
libgcc/unwind-dw2.c | 19 ++-
libgcc/unwind-dw2.h | 17 ++-
4 files changed, 175 insertions(+), 49 deletions(-)
diff --git a/libgcc/config/aarch64/aarch64-unwind.h
b/libgcc/config/aarch64/aarch64-unwind.h
index daf96624b5e..cc225a7e207 100644
--- a/libgcc/config/aarch64/aarch64-unwind.h
+++ b/libgcc/config/aarch64/aarch64-unwind.h
@@ -25,55 +25,155 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
If not, see
#if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
#define AARCH64_UNWIND_H
-#define DWARF_REGNUM_AARCH64_RA_STATE 34
+#include "ansidecl.h"
+#include <stdbool.h>
+
+#define AARCH64_DWARF_REGNUM_RA_STATE 34
+#define AARCH64_DWARF_RA_STATE_MASK 0x1
+
+/* The diversifiers used to sign a function's return address. */
+typedef enum
+{
+ AARCH64_RA_no_signing = 0x0,
+ AARCH64_RA_signing_SP = 0x1,
+} __attribute__((packed)) aarch64_RA_signing_method_t;
+
+/* The key used to sign a function's return address. */
+typedef enum {
+ AARCH64_PAUTH_KEY_A,
+ AARCH64_PAUTH_KEY_B,
+} __attribute__((packed)) aarch64_pointer_auth_key;
+
+#define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
+ aarch64_cie_aug_handler(fs, aug)
+
+#define MD_ARCH_EXTENSION_FRAME_INIT(context, fs) \
+ aarch64_arch_extension_frame_init(context, fs)
#define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
aarch64_demangle_return_addr (context, fs, addr)
-static inline int
-aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
+static inline aarch64_RA_signing_method_t
+aarch64_context_RA_state_get(struct _Unwind_Context *context)
+{
+ const int index = AARCH64_DWARF_REGNUM_RA_STATE;
+ return _Unwind_GetGR (context, index) & AARCH64_DWARF_RA_STATE_MASK;
+}
+
+static inline aarch64_RA_signing_method_t
+aarch64_fs_RA_state_get(_Unwind_FrameState const* fs)
{
- const struct dwarf_fde *fde = _Unwind_Find_FDE (context->bases.func,
- &context->bases);
- if (fde != NULL)
+ const int index = AARCH64_DWARF_REGNUM_RA_STATE;
+ return fs->regs.reg[index].loc.offset & AARCH64_DWARF_RA_STATE_MASK;
+}
+
+static inline void
+aarch64_fs_RA_state_set(_Unwind_FrameState *fs,
+ aarch64_RA_signing_method_t signing_method)
+{
+ fs->regs.reg[AARCH64_DWARF_REGNUM_RA_STATE].loc.offset = signing_method;
+}
+
+static inline void
+aarch64_fs_RA_state_toggle(_Unwind_FrameState *fs)
+{
+ aarch64_RA_signing_method_t signing_method = aarch64_fs_RA_state_get(fs);
+ gcc_assert (signing_method == AARCH64_RA_no_signing ||
+ signing_method == AARCH64_RA_signing_SP);
+ aarch64_fs_RA_state_set(fs, (signing_method == AARCH64_RA_no_signing)
+ ? AARCH64_RA_signing_SP
+ : AARCH64_RA_no_signing);
+}
+
+/* CIE handler for custom augmentation string. */
+static inline bool
+aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
+{
+ // AArch64 B-key pointer authentication.
+ if (aug == 'B')
{
- const struct dwarf_cie *cie = get_cie (fde);
- if (cie != NULL)
- {
- const unsigned char *aug_str = cie->augmentation;
- return __builtin_strchr ((const char *) aug_str,
- 'B') == NULL ? 0 : 1;
- }
+ fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
+ return true;
}
- return 0;
+ return false;
}
-/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
- unwind frame info FS. If ADDR_WORD is signed, we do address authentication
- on it using CFA of current frame. */
+/* At the entrance of a new frame, some cached information from the CIE/FDE,
+ * and registers values related to architectural extensions require a default
+ * initialization.
+ * If any of those values related to architecture extensions had to be saved
+ * for the next frame, it should be done via the architecture extensions
handler
+ * MD_FROB_UPDATE_CONTEXT in uw_update_context_1 (libgcc/unwind-dw2.c). */
+static inline void
+aarch64_arch_extension_frame_init (struct _Unwind_Context *context
ATTRIBUTE_UNUSED,
+ _Unwind_FrameState *fs)
+{
+ // Reset previously cached CIE/FDE information.
+ fs->regs.signing_key = AARCH64_PAUTH_KEY_A;
How about making the comment:
/* By default, DW_CFA_AARCH64_negate_ra_state assumes key A is being used
for signing. This can be overridden by adding 'B' to the augmentation
string. */
(if you agree that's a reasonable summary).
+
+ // Reset some registers.
+ // Note: the associated fs->how table is automatically reset to REG_UNSAVED
in
+ // uw_frame_state_for (libgcc/unwind-dw2.c). REG_UNSAVED means that whatever
+ // was in the previous context is the current register value. In other words,
+ // the next context inherits by default the previous value for a register.
+ // To keep this logic coherent with some architecture extensions, we need to
+ // reinitialize fs->how for some registers to REG_ARCHEXT.
How about being a bit more specific about the intent:
/* All registers are initially in state REG_UNSAVED, which indicates that
they inherit register values from the previous frame. However, the
return address starts every frame in the "unsigned" state. It also
starts every frame in a state that supports the original toggle-based
DW_CFA_AARCH64_negate_ra_state method of controlling RA signing. */
+ const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
+ fs->regs.how[reg] = REG_ARCHEXT;
Very minor, but I think this would be easier to read without the temporary.
+ aarch64_fs_RA_state_set(fs, AARCH64_RA_no_signing);
+}
+/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
+ unwind frame info FS. If ADDR_WORD is signed, we do address authentication
+ on it using CFA of current frame.
+ Note: this function is called after uw_update_context_1 in
uw_update_context.
+ uw_update_context_1 is the function in which val expression are computed.
Thus
expressions
+ if DW_CFA_val_expression is used, the value of the RA state register is
stored
+ in CONTEXT, not FS. (more details about when DW_CFA_val_expression is used
in
+ the next comment below) */
static inline void *
aarch64_demangle_return_addr (struct _Unwind_Context *context,
_Unwind_FrameState *fs,
_Unwind_Word addr_word)
{
void *addr = (void *)addr_word;
- const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
-
- if (fs->regs.how[reg] == REG_UNSAVED)
- return addr;
+ const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
+
+ // In libgcc, REG_ARCHEXT means that the RA state register was set by an
AArch64
+ // DWARF instruction and contains a valid value.
It's also used to describe the initial state.
+ // Return-address signing state is normally toggled by
DW_CFA_AARCH64_negate_ra_state
+ // (also knwon by its alias as DW_CFA_GNU_window_save).
+ // However, RA state register can be set directly via DW_CFA_val_expression
+ // too. GCC does not generate such CFI but some other compilers reportedly
do.
nit: drop the trailing full stop, since the following line continues
the sentence.
+ // (see PR104689 for more details).
+ // Any other value than REG_ARCHEXT should be interpreted as if the RA state
register
+ // is set by another DWARF instruction, and the value is fetchable via
_Unwind_GetGR.
+ aarch64_RA_signing_method_t signing_method = AARCH64_RA_no_signing;
+ if (fs->regs.how[reg] == REG_ARCHEXT)
+ {
+ signing_method = aarch64_fs_RA_state_get(fs);
+ }
+ else if (fs->regs.how[reg] != REG_UNSAVED)
+ {
+ signing_method = aarch64_context_RA_state_get(context);
+
+ // If the value was set in context, CONTEXT->by_value was set to 1.
+ // uw_install_context_1 contains an assert on by_value, to check that all
+ // registers values have been resolved before installing the context.
+ // This will make the unwinding crash if we don't reset
CONTEXT->by_value,
+ // and CONTEXT->reg[reg].
+ context->reg[reg] = _Unwind_Get_Unwind_Context_Reg_Val(0x0);
+ context->by_value[reg] = 0;
Is this change to the context necessary for the refactor, or is it
instead needed for the follow-on patches? If it's part of the refactor,
could you explain why it's needed now but wasn't before?
This is the only part that didn't look like a no-op change.
+ }
- /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
- REG_UNSAVED/REG_UNSAVED_ARCHEXT means RA signing is disabled/enabled),
- or set by a DW_CFA_expression. */
- if (fs->regs.how[reg] == REG_UNSAVED_ARCHEXT
- || (_Unwind_GetGR (context, reg) & 0x1) != 0)
+ if (signing_method == AARCH64_RA_signing_SP)
{
_Unwind_Word salt = (_Unwind_Word) context->cfa;
- if (aarch64_cie_signed_with_b_key (context) != 0)
+ if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
return __builtin_aarch64_autib1716 (addr, salt);
return __builtin_aarch64_autia1716 (addr, salt);
}
+ // else {} // signing_method == AARCH64_RA_no_signing, nothing to do.
IMO it'd be clearer without this line. Alternatively, we could make it:
else if (signing_method == AARCH64_RA_no_signing)
return addr;
gcc_unreachable ();
}
diff --git a/libgcc/unwind-dw2-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
index a6b249f9ad6..2ea78c4ef8d 100644
--- a/libgcc/unwind-dw2-execute_cfa.h
+++ b/libgcc/unwind-dw2-execute_cfa.h
@@ -271,23 +271,33 @@
fs->regs.how[reg] = REG_SAVED_VAL_EXP;
fs->regs.reg[reg].loc.exp = insn_ptr;
}
+ // Don't execute the expression, but jump over it by adding
+ // DW_FORM_block's size to insn_ptr.
insn_ptr = read_uleb128 (insn_ptr, &utmp);
insn_ptr += utmp;
break;
- case DW_CFA_GNU_window_save:
#if defined (__aarch64__) && !defined (__ILP32__)
- /* This CFA is multiplexed with Sparc. On AArch64 it's used to toggle
- return address signing status. REG_UNSAVED/REG_UNSAVED_ARCHEXT
- mean RA signing is disabled/enabled. */
- reg = DWARF_REGNUM_AARCH64_RA_STATE;
- gcc_assert (fs->regs.how[reg] == REG_UNSAVED
- || fs->regs.how[reg] == REG_UNSAVED_ARCHEXT);
- if (fs->regs.how[reg] == REG_UNSAVED)
- fs->regs.how[reg] = REG_UNSAVED_ARCHEXT;
- else
- fs->regs.how[reg] = REG_UNSAVED;
+ case DW_CFA_AARCH64_negate_ra_state:
+ // This CFA is multiplexed with Sparc.
+ // On AArch64 it's used to toggle the status of return address signing
+ // with SP as a diversifier.
+ // - REG_ARCHEXT means that the RA state register in FS contains a
+ // valid value, and that no other DWARF directive has changed the
value
+ // of this register.
+ // - any other value is not compatible with negating the RA state.
+ reg = AARCH64_DWARF_REGNUM_RA_STATE;
+
+ // /!\ Mixing DW_CFA_val_expression with
DW_CFA_AARCH64_negate_ra_state
+ // will result in an undefined behavior (likely an unwinding failure),
+ // as the chronology of the DWARF directives will be broken.
+ gcc_assert (fs->regs.how[reg] == REG_ARCHEXT);
Could we move the assert into aarch64_fs_RA_state_toggle, or do later
patches not want that?
+
+ // Toggle current state
+ aarch64_fs_RA_state_toggle(fs);
+ break;
#else
+ case DW_CFA_GNU_window_save:
/* ??? Hardcoded for SPARC register window configuration. */
if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
for (reg = 16; reg < 32; ++reg)
@@ -295,8 +305,8 @@
fs->regs.how[reg] = REG_SAVED_OFFSET;
fs->regs.reg[reg].loc.offset = (reg - 16) * sizeof (void *);
}
-#endif
break;
+#endif
case DW_CFA_GNU_args_size:
insn_ptr = read_uleb128 (insn_ptr, &utmp);
diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
index 0849e89cd34..fb60853825d 100644
--- a/libgcc/unwind-dw2.c
+++ b/libgcc/unwind-dw2.c
@@ -501,11 +501,11 @@ extract_cie_info (const struct dwarf_cie *cie, struct
_Unwind_Context *context,
fs->signal_frame = 1;
aug += 1;
}
- /* aarch64 B-key pointer authentication. */
- else if (aug[0] == 'B')
- {
- aug += 1;
- }
+
+#if defined(MD_ARCH_EXTENSION_CIE_AUG_HANDLER)
+ else if (MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug[0]))
+ aug += 1;
+#endif
/* Otherwise we have an unknown augmentation string.
Bail unless we saw a 'z' prefix. */
@@ -996,6 +996,9 @@ uw_frame_state_for (struct _Unwind_Context *context,
_Unwind_FrameState *fs)
memset (&fs->regs.how[0], 0,
sizeof (*fs) - offsetof (_Unwind_FrameState, regs.how[0]));
+#if defined(MD_ARCH_EXTENSION_FRAME_INIT)
+ MD_ARCH_EXTENSION_FRAME_INIT(context, fs);
+#endif
context->args_size = 0;
context->lsda = 0;
@@ -1197,7 +1200,11 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
{
case REG_UNSAVED:
case REG_UNDEFINED:
- case REG_UNSAVED_ARCHEXT:
+ case REG_ARCHEXT: // If the value depends on an augmenter, then there is
+ // no processing to do here, and the value computation
+ // should be delayed until the architecture handler
+ // computes the value correctly based on the augmenter
+ // information.
This would more usually be written as a comment above the case, rather than
to the right.
break;
case REG_SAVED_OFFSET:
diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
index 0dd8611f697..b75f4c65f98 100644
--- a/libgcc/unwind-dw2.h
+++ b/libgcc/unwind-dw2.h
@@ -22,16 +22,17 @@
see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
<http://www.gnu.org/licenses/>. */
-enum {
+enum register_rule
+{
REG_UNSAVED,
REG_SAVED_OFFSET,
REG_SAVED_REG,
REG_SAVED_EXP,
REG_SAVED_VAL_OFFSET,
REG_SAVED_VAL_EXP,
- REG_UNSAVED_ARCHEXT, /* Target specific extension. */
+ REG_ARCHEXT, /* Target specific extension. */
REG_UNDEFINED
-};
+} __attribute__((packed));
/* The result of interpreting the frame unwind info for a frame.
This is all symbolic at this point, as none of the values can
@@ -49,7 +50,7 @@ typedef struct
const unsigned char *exp;
} loc;
} reg[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
- unsigned char how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
+ enum register_rule how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
enum {
CFA_UNSET,
@@ -65,6 +66,14 @@ typedef struct
_Unwind_Sword cfa_offset;
_Unwind_Word cfa_reg;
const unsigned char *cfa_exp;
+
+ /* Architecture extensions information from CIE/FDE.
+ * Note: those information have to be saved in struct frame_state_reg_info
this information (singular)
Thanks for doing this. It's definitely a cleaner structure than what
we had before.
Richard
+ * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
+ * restore them. */
+#if defined(__aarch64__) && !defined (__ILP32__)
+ unsigned char signing_key;
+#endif
} regs;
/* The PC described by the current frame state. */
