[PATCH v2 0/3] objtool: Extend CFA updating/checking

[PATCH v2 0/3] objtool: Extend CFA updating/checking

[Julien Thierry]

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

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

[Julien Thierry]

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 <jthierry@redhat.com>
---
 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(&regs[CFI_BP],
+						-cfa->offset + STACKFRAME_SIZE,
+						STACKFRAME_BP_OFFSET)) {
 
 				/* mov %rsp, %rbp */
 				cfa->base = op->dest.reg;
-- 
2.25.4

[PATCH v2 2/3] objtool: check: Support addition to set CFA base

[Julien Thierry]

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 <jthierry@redhat.com>
---
 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(&regs[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

[PATCH v2 3/3] objtool: check: Make SP memory operation match PUSH/POP semantics

[Julien Thierry]

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 <jthierry@redhat.com>
---
 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

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

[Josh Poimboeuf]

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

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

[Julien Thierry]

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

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

[Josh Poimboeuf]

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

[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