Hi,
The following patches are the result of limitation found on the CFA
management code when trying to validate arm64 frames. I tried to keep
things simple and not contradict current CFA management logic nor
introduce too many corner cases.
Changes since V1[1]:
- Minor cleanups/rewording from discussion with Josh
[1] https://www.spinics.net/lists/kernel/msg3662146.html
Thanks,
Julien
-->
Julien Thierry (3):
objtool: check: Fully validate the stack frame
objtool: check: Support addition to set CFA base
objtool: check: Make SP memory operation match PUSH/POP semantics
tools/objtool/arch/x86/include/cfi_regs.h | 3 ++
tools/objtool/cfi.h | 2 +
tools/objtool/check.c | 54 +++++++++++++++++++++--
3 files changed, 55 insertions(+), 4 deletions(-)
--
2.25.4
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 <[email protected]>
---
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_SIZE 16
+
#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 = &state->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(&cfi->regs[CFI_BP],
+ -cfi->cfa.offset + STACKFRAME_SIZE,
+ STACKFRAME_BP_OFFSET) &&
+ check_reg_frame_pos(&cfi->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(®s[CFI_BP],
+ -cfa->offset + STACKFRAME_SIZE,
+ STACKFRAME_BP_OFFSET)) {
/* mov %rsp, %rbp */
cfa->base = op->dest.reg;
--
2.25.4
Architectures without PUSH/POP instructions will always access the stack
though memory operations (SRC/DEST_INDIRECT). Make those operations have
the same effect on the CFA as PUSH/POP, with no stack pointer
modification.
Signed-off-by: Julien Thierry <[email protected]>
---
tools/objtool/check.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9f7a14a24a65..724293fd1f59 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2008,6 +2008,14 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
break;
case OP_SRC_REG_INDIRECT:
+ if (!cfi->drap && op->dest.reg == cfa->base &&
+ op->dest.reg == CFI_BP) {
+
+ /* mov disp(%rsp), %rbp */
+ cfa->base = CFI_SP;
+ cfa->offset = cfi->stack_size;
+ }
+
if (cfi->drap && op->src.reg == CFI_BP &&
op->src.offset == cfi->drap_offset) {
@@ -2029,6 +2037,12 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
/* mov disp(%rbp), %reg */
/* mov disp(%rsp), %reg */
restore_reg(cfi, op->dest.reg);
+
+ } else if (op->src.reg == CFI_SP &&
+ op->src.offset == regs[op->dest.reg].offset + cfi->stack_size) {
+
+ /* mov disp(%rsp), %reg */
+ restore_reg(cfi, op->dest.reg);
}
break;
@@ -2106,6 +2120,12 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
/* mov reg, disp(%rsp) */
save_reg(cfi, op->src.reg, CFI_CFA,
op->dest.offset - cfi->cfa.offset);
+
+ } else if (op->dest.reg == CFI_SP) {
+
+ /* mov reg, disp(%rsp) */
+ save_reg(cfi, op->src.reg, CFI_CFA,
+ op->dest.offset - cfi->stack_size);
}
break;
--
2.25.4
On arm64, the compiler can set the frame pointer either
with a move operation or with and add operation like:
add (SP + constant), BP
For a simple move operation, the CFA base is changed from SP to BP.
Handle also changing the CFA base when the frame pointer is set with
an addition instruction.
Signed-off-by: Julien Thierry <[email protected]>
---
tools/objtool/check.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 50b3a4504db1..9f7a14a24a65 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1901,6 +1901,19 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
break;
}
+ if (!cfi->drap && op->src.reg == CFI_SP &&
+ op->dest.reg == CFI_BP && cfa->base == CFI_SP &&
+ check_reg_frame_pos(®s[CFI_BP],
+ -cfa->offset + op->src.offset + STACKFRAME_SIZE,
+ STACKFRAME_BP_OFFSET)) {
+
+ /* lea disp(%rsp), %rbp */
+ cfa->base = CFI_BP;
+ cfa->offset -= op->src.offset;
+ cfi->bp_scratch = false;
+ break;
+ }
+
if (op->src.reg == CFI_SP && cfa->base == CFI_SP) {
/* drap: lea disp(%rsp), %drap */
--
2.25.4
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
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
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
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