[PATCH 3/6] gas: aarch64: ginsn: add safeguards for reg-based stack pointer update
Alice Carlotti
alice.carlotti@arm.com
Thu Jun 5 10:51:37 GMT 2025
On Thu, Aug 08, 2024 at 08:38:50AM -0700, Indu Bhagat wrote:
> On aarch64, a user may update the stack pointer using a value previously
> updated in a register, e.g.,
>
> ...[C1]...
> mov x16, 4128
> ...[C2]...
> add/sub sp, sp, x16
> ...[C3]...
>
> (Such patterns are informally referred to as SPLIT_SP_UPDATE code
> patterns in the implementation.)
>
> The SCFI machinery currently does not handle these code patterns. It
> does not track values or the data flow in these code patterns; So, it
> bails out and issues an error:
> "Error: SCFI: unsupported stack manipulation pattern"
>
> Moving forward, we will later add such capability to the SCFI machinery (in a
> subsequent patch). Supporting SPLIT_SP_UPDATE will make SCFI more useful
> as such code patterns seem prevalent in aarch64 targets. That said,
> SCFI will support only those SPLIT_SP_UPDATE patterns where no control
> flow operation is present in C2 (This will be enforced by the SCFI
> machinery later).
>
> Note that, the ginsns for such mov and add/sub aarch64 instructions are
> already being created. But currently, the ginsn creation machinery does
> not include enough checks for safeguarding against incorrect SCFI in
> presence of such code patterns. Specifically at the ginsn creation
> time, we need to ensure:
> - No writes to the involved register between the mov and add/sub go
> unnoticed. In the simple code example above, no writes to x16 must
> go unnoticed in code block C2, as it impacts the traceability of
> REG_SP.
>
> This will allow SCFI machinery to continue to guarantee that:
> - a non-constant increment/decrement to REG_SP make CFA untraceable
> and hence, are disallowed for SCFI purposes.
> - constant increment/decrement to REG_SP do not make CFA untraceable.
>
> The ginsn creation machinery in the aarch64 backend now uses a new
> static variable of type struct split_sp_state. This allows to keep
> track of the following:
> - (#1) Registers the sight of every 'mov reg, imm' insn.
> - (#2) If any insn writes to 'reg' _and_ no ginsn is created for it,
> the machinery now emits a GINSN_TYPE_OTHER with dest=reg. To do
> this, the whitelisting logic needs to be made aware of the special
> case.
> - (#3) If a subsequent 'add sp, sp, reg' is now seen, the ginsn
> creation goes on as normal.
>
> Note that, the ginsn creation process cannot possibly know if an
> applicable mov op will go on to serve an eventual stack update insn.
> This will remain the job of the SCFI machinery. So, the ginsn creation
> process _must_ treat all occurrences of a mov op equally.
>
> In other words, the ginsn creation process should simply make sure that
> all write-after-write dependencies for all the interesting registers are
> reflected in ginsns. Finally, regarding when is the right time to reset
> the split_sp_state - currently, this is done:
> - at each code block begin boundary via hooking on to the
> TC_GINSN_DATA_INIT target-specific implementation.
> Arguably, the split_sp_state could also be reset when a change of flow
> instruction is seen (as later SCFI will implement the same restriction),
> but the current implementation in this patch choses to not do so.
>
> gas/
> * config/tc-aarch64-ginsn.c (AARCH64_MAX_INT_DWARF_REG): New
> definition.
> (struct split_sp_update): Likewise.
> (split_sp_update_reg_match_p): Likewise.
> (split_sp_update_add): Likewise.
> (split_sp_update_reset_reg): Likewise.
> (split_sp_update_reset_all): Likewise.
> (aarch64_ginsn_data_init): Likewise.
> (aarch64_ginsn_mov_imm): Add reg to split_sp_upate state.
> (aarch64_ginsn_safe_to_skip_p): Adjust whitelisting logic.
> (aarch64_ginsn_unhandled): A skipped ginsn with a WAW dependence
> on a reg involved in split sp update must lead to generation of
> GINSN_TYPE_OTHER.
> * config/tc-aarch64.h (aarch64_ginsn_data_init): New
> declaration.
> (TC_GINSN_DATA_INIT): New definition.
> ---
> gas/config/tc-aarch64-ginsn.c | 113 +++++++++++++++++++++++++++++++++-
> gas/config/tc-aarch64.h | 3 +
> 2 files changed, 115 insertions(+), 1 deletion(-)
>
> diff --git a/gas/config/tc-aarch64-ginsn.c b/gas/config/tc-aarch64-ginsn.c
> index 0dce2a71bd9..4eab74afc5d 100644
> --- a/gas/config/tc-aarch64-ginsn.c
> +++ b/gas/config/tc-aarch64-ginsn.c
> @@ -26,6 +26,37 @@
>
> /* Invalid DWARF register number. Used when WZR / XZR is seen. */
> #define GINSN_DW2_REGNUM_INVALID (~0U)
> +/* The maximum DWARF register number for any integer register. */
> +#define AARCH64_MAX_INT_DWARF_REG REG_SP
> +
> +/* Structure for additional book-keeping for code patterns where reg-based
> + REG_SP update is done, e.g.,
> + (A) mov r16, 4128
> + (B) add sp, sp, r16
> + This is informally referred to as SPLIT_SP_UPDATE code pattern in the
> + implementation. Such book-keeping is necessary to ensure all
> + write-after-write dependencies on x16, atleast between (A) and (B) are
> + reflected in generated ginsns. In other words, this means:
> + - correctly whitelist: an instruction otherwise deemed uninteresting for
> + SCFI purposes, now should be considered necessary (because it writes to
> + x16)
> + - correctly create: add appropriate ginsns to convey the WAW dependence.
> + The above two items are somewhat correlated but verbosity here helps
> + underline that such book-keeping is necessary to ensure SCFI
> + correctness. */
> +
> +struct split_sp_update
> +{
> + /* Whether a mov imm op to a reg has been seen. */
> + bool mov_p;
This boolean seems unnecessary - it looks like it just indicates whether
movreg_mask is nonzero, but we can just test movreg_mask directly.
> + /* Bitfield to book-keep the DWARF register numbers involved in the
> + SPLIT_SP_UPDATE code pattern. Only integer registers are expected for
> + such patterns. Writers to movreg_mask must mind AARCH64_MAX_INT_DWARF_REG
> + limit. */
> + unsigned int movreg_mask;
> +};
> +
> +static struct split_sp_update split_sp;
>
> /* Return whether the given register number is a callee-saved register for
> SCFI purposes.
> @@ -97,6 +128,65 @@ ginsn_dw2_regnum_invalid_p (unsigned int opnd_reg)
> return (opnd_reg == GINSN_DW2_REGNUM_INVALID);
> }
>
> +/* Return whether the given REG matches with the movreg_mask in
> + the SPLIT_SP_UPDATE pattern. */
> +
> +static bool
> +split_sp_update_reg_match_p (unsigned int reg)
> +{
> + bool match_p = false;
> +
> + if (!split_sp.mov_p)
> + return false;
> +
> + if (reg <= AARCH64_MAX_INT_DWARF_REG && (split_sp.movreg_mask & (1 << reg)))
> + match_p = true;
> +
> + return match_p;
> +}
> +
> +/* Add REG to the set of tracked registers involved in a potential
> + SPLIT_SP_UPDATE update pattern. */
> +
> +static void
> +split_sp_update_add (unsigned int reg)
> +{
> + /* The dataype of movreg_mask is unsigned int. */
> + if (reg <= AARCH64_MAX_INT_DWARF_REG)
> + {
> + split_sp.mov_p = true;
> + split_sp.movreg_mask |= (1 << reg);
> + }
> +}
> +
> +/* Reset the given REG from the set of tracked registers potentially involved
> + in SPLIT_SP_UPDATE pattern. */
> +
> +static void
> +split_sp_update_reset_reg (unsigned int reg)
> +{
> + /* The dataype of movreg_mask is unsigned int. */
> + if (reg <= AARCH64_MAX_INT_DWARF_REG)
> + split_sp.movreg_mask &= ~(1 << reg);
> + if (!split_sp.movreg_mask)
> + split_sp.mov_p = false;
> +}
> +
> +/* Reset all state related to SPLIT_SP_UPDATE pattern. */
> +
> +static void
> +split_sp_update_reset_all (void)
> +{
> + split_sp.mov_p = false;
> + split_sp.movreg_mask = 0;
> +}
> +
> +void
> +aarch64_ginsn_data_init (void)
> +{
> + split_sp_update_reset_all ();
> +}
> +
> /* Generate ginsn for addsub instructions with immediate opnd. */
>
> static ginsnS *
> @@ -634,6 +724,10 @@ aarch64_ginsn_mov_imm (const symbolS *insn_end_sym)
> {
> src_imm = inst.reloc.exp.X_add_number;
> src_type = GINSN_SRC_IMM;
> + /* Only integer registers are expected here. */
> + gas_assert (dst_reg <= AARCH64_MAX_INT_DWARF_REG);
> + /* Update the state now. */
> + split_sp_update_add (dst_reg);
> }
> else
> /* Skip now and handle via aarch64_ginsn_unhandled () code path. */
> @@ -703,6 +797,17 @@ aarch64_ginsn_safe_to_skip_p (void)
> break;
> }
>
> + /* Irrespective of iclass, if SPLIT_SP_UPDATE is currently ongoing
> + and destination reg matches a previously seen mov reg, skipping
> + this will affect SCFI correctness. Don't. */
> + if (!skip_p && opnd && (opnd->type == AARCH64_OPND_Rd
> + || opnd->type == AARCH64_OPND_Rd_SP))
> + {
This condition looks wrong - the '!skip_p' check makes the following assignment
a noop.
Additionally, there are some cases where a later operand is a destination - for
example, ldp has two destination operands, and ldadd has its register
destination as the second operand. Are these cases already covered elsewhere?
> + opnd_reg = ginsn_dw2_regnum (opnd);
> + if (split_sp_update_reg_match_p (opnd_reg))
> + skip_p = false;
> + }
> +
> return skip_p;
> }
>
> @@ -800,7 +905,8 @@ aarch64_ginsn_unhandled (void)
> }
>
> /* Finally, irrespective of the iclass, check if the missed instructions are
> - affecting REG_SP or REG_FP. */
> + affecting REG_SP or REG_FP. Also, check if the SPLIT_SP_UPDATE pattern is
> + in effect and destination reg matches. */
> else if (dest && (dest->type == AARCH64_OPND_Rd
> || dest->type == AARCH64_OPND_Rd_SP))
> {
> @@ -808,6 +914,11 @@ aarch64_ginsn_unhandled (void)
>
> if (dw2_regnum == REG_SP || dw2_regnum == REG_FP)
> err = AARCH64_GINSN_UNHANDLED_DEST_REG;
> + else if (split_sp_update_reg_match_p (dw2_regnum))
> + {
> + err = AARCH64_GINSN_UNHANDLED_DEST_REG;
> + split_sp_update_reset_reg (dw2_regnum);
> + }
> }
>
> return err;
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 15e22436bf7..67561cd0d5f 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -284,6 +284,9 @@ extern void aarch64_after_parse_args (void);
> #define SCFI_CALLEE_SAVED_REG_P(dw2reg) aarch64_scfi_callee_saved_p (dw2reg)
> extern bool aarch64_scfi_callee_saved_p (uint32_t dw2reg_num);
>
> +extern void aarch64_ginsn_data_init (void);
> +#define TC_GINSN_DATA_INIT aarch64_ginsn_data_init
> +
> /* Whether SFrame stack trace info is supported. */
> extern bool aarch64_support_sframe_p (void);
> #define support_sframe_p aarch64_support_sframe_p
> --
> 2.43.0
>
More information about the Binutils
mailing list