Messages in this thread | | | Subject | Re: [PATCH v2 1/3] objtool: check: Fully validate the stack frame | From | Julien Thierry <> | Date | Mon, 12 Oct 2020 11:21:49 +0100 |
| |
On 9/29/20 8:18 PM, Josh Poimboeuf wrote: > On Mon, Sep 28, 2020 at 10:36:29AM +0100, Julien Thierry wrote: >> +++ b/tools/objtool/arch/x86/include/cfi_regs.h >> @@ -22,4 +22,7 @@ >> #define CFI_RA 16 >> #define CFI_NUM_REGS 17 > > A few more naming nitpicks: > >> +#define STACKFRAME_BP_OFFSET -16 >> +#define STACKFRAME_RA_OFFSET -8 > > "Stack frame" has more than one meaning now, I suppose. i.e. it could > also include the callee-saved registers and any other stack space > allocated by the function. > > Would "call frame" be clearer? > > CALL_FRAME_BP_OFFSET > CALL_FRAME_RA_OFFSET > > ?
I would've thought that the call-frame could include the stackframe + other callee saved regs. Whereas stackframe tends to used for the caller's frame pointer + return address (i.e. what allows unwinding). Unless I'm getting lost with things.
And if call frame is associated with the region starting from the stack pointer at the parent call point (since this is what CFA is), then it shouldn't be associated with the framepointer + return address structure since this could be anywhere on the call frame (not at a fixed offset) as long as the new frame pointer points to the structure.
> >> +++ b/tools/objtool/cfi.h >> @@ -35,4 +35,6 @@ struct cfi_state { >> bool end; >> }; >> >> +#define STACKFRAME_SIZE 16 > > CALL_FRAME_SIZE ? > > I'm sort of contradicting my previous comment here, but even though this > value may be generic, it's also very much intertwined with the > CALL_FRAME_{BP|RA}_OFFSET values. So I get the feeling it really > belongs in the arch-specific cfi_regs.h next to the other defines after > all. >
Agreed.
-- Julien Thierry
| |