[PATCH] gdb/arm: Cleanup of arm_m_exception_cache
Luis Machado
luis.machado@arm.com
Tue Aug 9 13:02:29 GMT 2022
Hi,
On 8/8/22 08:56, Torbjörn SVENSSON via Gdb-patches wrote:
> With this change, only valid content of LR is accepted for the current
> target. If the content for LR is anything but EXC_RETURN or FNC_RETURN
> will cause GDB to assert since it's an invalid state for the unwinder.
> FNC_RETURN pattern requires Security Extensions to be enabled or GDB
> will assert due to the bad state of the unwinder.
If we have corrupt data, do we risk running into this assertion?
>
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
> gdb/arm-tdep.c | 343 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 183 insertions(+), 160 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 7d8d040f8f1..ad5ada39aea 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3345,19 +3345,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
> {
> struct gdbarch *gdbarch = get_frame_arch (this_frame);
> arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
> - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> struct arm_prologue_cache *cache;
> CORE_ADDR lr;
> CORE_ADDR sp;
> CORE_ADDR unwound_sp;
> - uint32_t sp_r0_offset = 0;
> - LONGEST xpsr;
> - uint32_t exc_return;
> + ULONGEST xpsr;
> + bool exc_return;
> bool fnc_return;
> - uint32_t extended_frame_used;
> - bool secure_stack_used = false;
> - bool default_callee_register_stacking = false;
> - bool exception_domain_is_secure = false;
>
> cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
> arm_cache_init (cache, this_frame);
> @@ -3380,9 +3374,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
> return cache;
> }
>
> + /* Check FNC_RETURN indicator bits (24-31). */
> fnc_return = (((lr >> 24) & 0xff) == 0xfe);
> - if (tdep->have_sec_ext && fnc_return)
> + if (fnc_return)
> {
> + /* FNC_RETURN is only valid for targets with Security Extension. */
> + gdb_assert (tdep->have_sec_ext);
> +
An assertion is a bit of a strong hand here. It will crash GDB, essentially. Should we go for an
error instead? That will stop the unwinder dead in its tracks and return.
Unwinders may get corrupt data, so it is hard to rule out issues, even if GDB is doing the right thing.
> if (!arm_unwind_secure_frames)
> {
> warning (_("Non-secure to secure stack unwinding disabled."));
> @@ -3428,6 +3426,14 @@ arm_m_exception_cache (struct frame_info *this_frame)
> exc_return = (((lr >> 24) & 0xff) == 0xff);
> if (exc_return)
> {
> + int sp_regnum;
> + uint32_t sp_r0_offset = 0;
> + bool extended_frame_used;
extended_frame_used could be defined further down or removed.
Since we're touching this code, it would be nice to get the variable declarations closer to
where they're used. This makes things cleaner now that we can do it.
> + bool secure_stack_used = false;
> + bool default_callee_register_stacking = false;
> + bool exception_domain_is_secure = false;
> + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +
> /* Check EXC_RETURN bit SPSEL if Main or Thread (process) stack used. */
> bool process_stack_used = ((lr & (1 << 2)) != 0);
>
> @@ -3455,188 +3461,205 @@ arm_m_exception_cache (struct frame_info *this_frame)
> {
> if (secure_stack_used)
> /* Secure thread (process) stack used, use PSP_S as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
> + sp_regnum = tdep->m_profile_psp_s_regnum;
> else
> /* Non-secure thread (process) stack used, use PSP_NS as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_ns_regnum);
> + sp_regnum = tdep->m_profile_psp_ns_regnum;
> }
> else
> {
> if (secure_stack_used)
> /* Secure main stack used, use MSP_S as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
> + sp_regnum = tdep->m_profile_msp_s_regnum;
> else
> /* Non-secure main stack used, use MSP_NS as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
> + sp_regnum = tdep->m_profile_msp_ns_regnum;
> }
> }
> else
> {
> if (process_stack_used)
> /* Thread (process) stack used, use PSP as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_regnum);
> + sp_regnum = tdep->m_profile_psp_regnum;
> else
> /* Main stack used, use MSP as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
> - }
> - }
> -
> - /* Fetch the SP to use for this frame. */
> - unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
> -
> - /* Exception entry context stacking are described in ARMv8-M (section B3.19)
> - and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference Manuals.
> -
> - The following figure shows the structure of the stack frame when Security
> - and Floating-point extensions are present.
> -
> - SP Offsets
> - Without With
> - Callee Regs Callee Regs
> - (Secure -> Non-Secure)
> - +-------------------+
> - 0xA8 | | 0xD0
> - +===================+ --+ <-- Original SP
> - 0xA4 | S31 | 0xCC |
> - +-------------------+ |
> - ... | Additional FP context
> - +-------------------+ |
> - 0x68 | S16 | 0x90 |
> - +===================+ --+
> - 0x64 | Reserved | 0x8C |
> - +-------------------+ |
> - 0x60 | FPSCR | 0x88 |
> - +-------------------+ |
> - 0x5C | S15 | 0x84 | FP context
> - +-------------------+ |
> - ... |
> - +-------------------+ |
> - 0x20 | S0 | 0x48 |
> - +===================+ --+
> - 0x1C | xPSR | 0x44 |
> - +-------------------+ |
> - 0x18 | Return address | 0x40 |
> - +-------------------+ |
> - 0x14 | LR(R14) | 0x3C |
> - +-------------------+ |
> - 0x10 | R12 | 0x38 | State context
> - +-------------------+ |
> - 0x0C | R3 | 0x34 |
> - +-------------------+ |
> - ... |
> - +-------------------+ |
> - 0x00 | R0 | 0x28 |
> - +===================+ --+
> - | R11 | 0x24 |
> - +-------------------+ |
> - ... |
> - +-------------------+ | Additional state context
> - | R4 | 0x08 | when transitioning from
> - +-------------------+ | Secure to Non-Secure
> - | Reserved | 0x04 |
> - +-------------------+ |
> - | Magic signature | 0x00 |
> - +===================+ --+ <-- New SP */
> -
> - /* With the Security extension, the hardware saves R4..R11 too. */
> - if (exc_return && tdep->have_sec_ext && secure_stack_used
> - && (!default_callee_register_stacking || exception_domain_is_secure))
> - {
> - /* Read R4..R11 from the integer callee registers. */
> - cache->saved_regs[4].set_addr (unwound_sp + 0x08);
> - cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
> - cache->saved_regs[6].set_addr (unwound_sp + 0x10);
> - cache->saved_regs[7].set_addr (unwound_sp + 0x14);
> - cache->saved_regs[8].set_addr (unwound_sp + 0x18);
> - cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
> - cache->saved_regs[10].set_addr (unwound_sp + 0x20);
> - cache->saved_regs[11].set_addr (unwound_sp + 0x24);
> - sp_r0_offset = 0x28;
> - }
> -
> - /* The hardware saves eight 32-bit words, comprising xPSR,
> - ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
> - "B1.5.6 Exception entry behavior" in
> - "ARMv7-M Architecture Reference Manual". */
> - cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
> - cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
> - cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
> - cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
> - cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x10);
> - cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x14);
> - cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x18);
> - cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x1C);
> -
> - /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
> - type used. */
> - extended_frame_used = ((lr & (1 << 4)) == 0);
> - if (exc_return && extended_frame_used)
> - {
> - int i;
> - int fpu_regs_stack_offset;
> - ULONGEST fpccr;
> -
> - /* Read FPCCR register. */
> - gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
> - ARM_INT_REGISTER_SIZE,
> - byte_order, &fpccr));
> - bool fpccr_ts = bit (fpccr,26);
> + sp_regnum = tdep->m_profile_msp_regnum;
> + }
>
> - /* This code does not take into account the lazy stacking, see "Lazy
> - context save of FP state", in B1.5.7, also ARM AN298, supported
> - by Cortex-M4F architecture.
> - To fully handle this the FPCCR register (Floating-point Context
> - Control Register) needs to be read out and the bits ASPEN and LSPEN
> - could be checked to setup correct lazy stacked FP registers.
> - This register is located at address 0xE000EF34. */
> + /* Set the active SP regnum. */
> + arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
>
> - /* Extended stack frame type used. */
> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x20;
> - for (i = 0; i < 8; i++)
> - {
> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
> - fpu_regs_stack_offset += 8;
> - }
> - cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x60);
> - fpu_regs_stack_offset += 4;
> + /* Fetch the SP to use for this frame. */
> + unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>
> - if (tdep->have_sec_ext && !default_callee_register_stacking && fpccr_ts)
> + /* Exception entry context stacking are described in ARMv8-M (section
> + B3.19) and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference
> + Manuals.
> +
> + The following figure shows the structure of the stack frame when
> + Security and Floating-point extensions are present.
> +
> + SP Offsets
> + Without With
> + Callee Regs Callee Regs
> + (Secure -> Non-Secure)
> + +-------------------+
> + 0xA8 | | 0xD0
> + +===================+ --+ <-- Original SP
> + 0xA4 | S31 | 0xCC |
> + +-------------------+ |
> + ... | Additional FP context
> + +-------------------+ |
> + 0x68 | S16 | 0x90 |
> + +===================+ --+
> + 0x64 | Reserved | 0x8C |
> + +-------------------+ |
> + 0x60 | FPSCR | 0x88 |
> + +-------------------+ |
> + 0x5C | S15 | 0x84 | FP context
> + +-------------------+ |
> + ... |
> + +-------------------+ |
> + 0x20 | S0 | 0x48 |
> + +===================+ --+
> + 0x1C | xPSR | 0x44 |
> + +-------------------+ |
> + 0x18 | Return address | 0x40 |
> + +-------------------+ |
> + 0x14 | LR(R14) | 0x3C |
> + +-------------------+ |
> + 0x10 | R12 | 0x38 | State context
> + +-------------------+ |
> + 0x0C | R3 | 0x34 |
> + +-------------------+ |
> + ... |
> + +-------------------+ |
> + 0x00 | R0 | 0x28 |
> + +===================+ --+
> + | R11 | 0x24 |
> + +-------------------+ |
> + ... |
> + +-------------------+ | Additional state
> + | R4 | 0x08 | context when
> + +-------------------+ | transitioning from
> + | Reserved | 0x04 | Secure to Non-Secure
> + +-------------------+ |
> + | Magic signature | 0x00 |
> + +===================+ --+ <-- New SP */
> +
> + /* With the Security extension, the hardware saves R4..R11 too. */
> + if (tdep->have_sec_ext && secure_stack_used
> + && (!default_callee_register_stacking || exception_domain_is_secure))
> + {
> + /* Read R4..R11 from the integer callee registers. */
> + cache->saved_regs[4].set_addr (unwound_sp + 0x08);
> + cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
> + cache->saved_regs[6].set_addr (unwound_sp + 0x10);
> + cache->saved_regs[7].set_addr (unwound_sp + 0x14);
> + cache->saved_regs[8].set_addr (unwound_sp + 0x18);
> + cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
> + cache->saved_regs[10].set_addr (unwound_sp + 0x20);
> + cache->saved_regs[11].set_addr (unwound_sp + 0x24);
> + sp_r0_offset = 0x28;
> + }
> +
> + /* The hardware saves eight 32-bit words, comprising xPSR,
> + ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
> + "B1.5.6 Exception entry behavior" in
> + "ARMv7-M Architecture Reference Manual". */
> + cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
> + cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
> + cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
> + cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
> + cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x10);
> + cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x14);
> + cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x18);
> + cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x1C);
> +
> + /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
> + type used. */
> + extended_frame_used = ((lr & (1 << 4)) == 0);
Declaring extended_frame_used here would be nice. That's the only place where it is
used.
Alternatively, you could check for bit 4 in the conditional expression below and add a
comment explaining this is checking if the extended frame is being used.
> + if (extended_frame_used)
> {
> - /* Handle floating-point callee saved registers. */
> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
> - for (i = 8; i < 16; i++)
> - {
> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
> + int i;
> + int fpu_regs_stack_offset;
> + ULONGEST fpccr;
> +
> + /* Read FPCCR register. */
> + gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
> + ARM_INT_REGISTER_SIZE,
> + byte_order, &fpccr));
> + bool fpccr_ts = bit (fpccr,26);
> +
> + /* This code does not take into account the lazy stacking, see "Lazy
> + context save of FP state", in B1.5.7, also ARM AN298, supported
> + by Cortex-M4F architecture.
> + To fully handle this the FPCCR register (Floating-point Context
> + Control Register) needs to be read out and the bits ASPEN and
> + LSPEN could be checked to setup correct lazy stacked FP registers.
> + This register is located at address 0xE000EF34. */
> +
> + /* Extended stack frame type used. */
> + fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x20;
> + for (i = 0; i < 8; i++)
> + {
> + cache->saved_regs[ARM_D0_REGNUM + i]
> + .set_addr (fpu_regs_stack_offset);
> fpu_regs_stack_offset += 8;
> }
> + cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp
> + + sp_r0_offset + 0x60);
> + fpu_regs_stack_offset += 4;
>
> - arm_cache_set_active_sp_value (cache, tdep,
> - unwound_sp + sp_r0_offset + 0xA8);
> + if (tdep->have_sec_ext && !default_callee_register_stacking
> + && fpccr_ts)
> + {
> + /* Handle floating-point callee saved registers. */
> + fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
> + for (i = 8; i < 16; i++)
> + {
> + cache->saved_regs[ARM_D0_REGNUM + i]
> + .set_addr (fpu_regs_stack_offset);
> + fpu_regs_stack_offset += 8;
> + }
> +
> + arm_cache_set_active_sp_value (cache, tdep,
> + unwound_sp + sp_r0_offset + 0xA8);
> + }
> + else
> + {
> + /* Offset 0x64 is reserved. */
> + arm_cache_set_active_sp_value (cache, tdep,
> + unwound_sp + sp_r0_offset + 0x68);
> + }
> }
> else
> {
> - /* Offset 0x64 is reserved. */
> + /* Standard stack frame type used. */
> arm_cache_set_active_sp_value (cache, tdep,
> - unwound_sp + sp_r0_offset + 0x68);
> + unwound_sp + sp_r0_offset + 0x20);
> }
> - }
> - else
> - {
> - /* Standard stack frame type used. */
> - arm_cache_set_active_sp_value (cache, tdep,
> - unwound_sp + sp_r0_offset + 0x20);
> - }
>
> - /* If bit 9 of the saved xPSR is set, then there is a four-byte
> - aligner between the top of the 32-byte stack frame and the
> - previous context's stack pointer. */
> - if (safe_read_memory_integer (unwound_sp + sp_r0_offset + 0x1C, 4,
> - byte_order, &xpsr)
> - && (xpsr & (1 << 9)) != 0)
> - arm_cache_set_active_sp_value (cache, tdep,
> - arm_cache_get_prev_sp_value (cache, tdep) + 4);
> + /* If bit 9 of the saved xPSR is set, then there is a four-byte
> + aligner between the top of the 32-byte stack frame and the
> + previous context's stack pointer. */
> + gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
> + ARM_PS_REGNUM].addr (), 4,
> + byte_order, &xpsr));
> + if ((xpsr & (1 << 9)) != 0)
> + {
> + CORE_ADDR new_sp = arm_cache_get_prev_sp_value (cache, tdep) + 4;
> + arm_cache_set_active_sp_value (cache, tdep, new_sp);
> + }
>
> - return cache;
> + return cache;
> + }
> +
> + gdb_assert_not_reached ("Invalid LR contet");
"content". Again, this will crash GDB. Should we error out instead?
> }
>
> /* Implementation of function hook 'this_id' in
More information about the Gdb-patches
mailing list