Re: [PATCH v2 1/3] objtool: check: Fully validate the stack frame
On 10/12/20 4:35 PM, Josh Poimboeuf wrote: On Mon, Oct 12, 2020 at 11:21:49AM +0100, Julien Thierry wrote: On 9/29/20 8:18 PM, Josh Poimboeuf wrote: "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. Hm, probably so. 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. I've always seen "stack frame" used to indicate the function's entire stack. 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. I suppose "call frame" and "stack frame" probably mean the same thing, in which case neither is appropriate here... In fact, maybe we could forget the concept of a frame (or even a struct) here. If cfa.base is CFI_BP, then is regs[CFI_BP].offset always the same as -cfa.offset? i.e. could the BP checks could it just be a simple regs[CFI_BP].offset == -cfa.offset check? I guess that makes sense. If the above was no true it would mean that BP is not pointing to the unwind information. And then is RA at regs[CFI_BP].offset + 8? In the case of aarch64, the saved frame pointer and return address appear in the same order as on x86_64. So that would work. If that can make things simpler for now I can go with that. Thanks for the suggestion. -- Julien Thierry
Re: [PATCH v2 1/3] objtool: check: Fully validate the stack frame
On Mon, Oct 12, 2020 at 11:21:49AM +0100, Julien Thierry wrote: > On 9/29/20 8:18 PM, Josh Poimboeuf wrote: > > "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. Hm, probably so. > 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. I've always seen "stack frame" used to indicate the function's entire stack. > 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. I suppose "call frame" and "stack frame" probably mean the same thing, in which case neither is appropriate here... In fact, maybe we could forget the concept of a frame (or even a struct) here. If cfa.base is CFI_BP, then is regs[CFI_BP].offset always the same as -cfa.offset? i.e. could the BP checks could it just be a simple regs[CFI_BP].offset == -cfa.offset check? And then is RA at regs[CFI_BP].offset + 8? -- Josh
Re: [PATCH v2 1/3] objtool: check: Fully validate the stack frame
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_RA16
#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
Re: [PATCH v2 1/3] objtool: check: Fully validate the stack frame
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
?
> +++ 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.
--
Josh
[PATCH v2 1/3] objtool: check: Fully validate the stack frame
A valid stack frame should contain both the return address and the
previous frame pointer value.
On x86, the return value is placed on the stack by the calling
instructions. On other architectures, the callee need to explicitly
save the return address on the stack.
Add the necessary checks to verify a function properly sets up all the
elements of the stack frame.
Signed-off-by: Julien Thierry
---
tools/objtool/arch/x86/include/cfi_regs.h | 3 +++
tools/objtool/cfi.h | 2 ++
tools/objtool/check.c | 21 +
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/arch/x86/include/cfi_regs.h
b/tools/objtool/arch/x86/include/cfi_regs.h
index 79bc517efba8..4e443b49d0d3 100644
--- a/tools/objtool/arch/x86/include/cfi_regs.h
+++ b/tools/objtool/arch/x86/include/cfi_regs.h
@@ -22,4 +22,7 @@
#define CFI_RA 16
#define CFI_NUM_REGS 17
+#define STACKFRAME_BP_OFFSET -16
+#define STACKFRAME_RA_OFFSET -8
+
#endif /* _OBJTOOL_CFI_REGS_H */
diff --git a/tools/objtool/cfi.h b/tools/objtool/cfi.h
index c7c59c6a44ee..2691b6ce4fcd 100644
--- a/tools/objtool/cfi.h
+++ b/tools/objtool/cfi.h
@@ -35,4 +35,6 @@ struct cfi_state {
bool end;
};
+#define STACKFRAME_SIZE16
+
#endif /* _OBJTOOL_CFI_H */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2df9f769412e..50b3a4504db1 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1668,12 +1668,24 @@ static bool has_modified_stack_frame(struct instruction
*insn, struct insn_state
return false;
}
+static bool check_reg_frame_pos(const struct cfi_reg *reg, int
stackframe_start,
+ int expected_offset)
+{
+ return reg->base == CFI_CFA &&
+ reg->offset == stackframe_start + expected_offset;
+}
+
static bool has_valid_stack_frame(struct insn_state *state)
{
struct cfi_state *cfi = >cfi;
- if (cfi->cfa.base == CFI_BP && cfi->regs[CFI_BP].base == CFI_CFA &&
- cfi->regs[CFI_BP].offset == -16)
+ if (cfi->cfa.base == CFI_BP && cfi->cfa.offset >= STACKFRAME_SIZE &&
+ check_reg_frame_pos(>regs[CFI_BP],
+ -cfi->cfa.offset + STACKFRAME_SIZE,
+ STACKFRAME_BP_OFFSET) &&
+ check_reg_frame_pos(>regs[CFI_RA],
+ -cfi->cfa.offset + STACKFRAME_SIZE,
+ STACKFRAME_RA_OFFSET))
return true;
if (cfi->drap && cfi->regs[CFI_BP].base == CFI_BP)
@@ -1802,8 +1814,9 @@ static int update_cfi_state(struct instruction *insn,
struct cfi_state *cfi,
case OP_SRC_REG:
if (op->src.reg == CFI_SP && op->dest.reg == CFI_BP &&
cfa->base == CFI_SP &&
- regs[CFI_BP].base == CFI_CFA &&
- regs[CFI_BP].offset == -cfa->offset) {
+ check_reg_frame_pos([CFI_BP],
+ -cfa->offset + STACKFRAME_SIZE,
+ STACKFRAME_BP_OFFSET)) {
/* mov %rsp, %rbp */
cfa->base = op->dest.reg;
--
2.25.4
