Re: [PATCH v2 1/3] objtool: check: Fully validate the stack frame

From: Julien Thierry
Date: Mon Oct 12 2020 - 06:21:59 EST




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