Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
hint macros to flag instructions which remove or modify return
addresses.
Signed-off-by: Alexandre Chartre <[email protected]>
---
arch/x86/include/asm/orc_types.h | 2 +
arch/x86/include/asm/unwind_hints.h | 23 +++++++++
tools/arch/x86/include/asm/orc_types.h | 2 +
tools/objtool/check.c | 67 ++++++++++++++++++++++++++
4 files changed, 94 insertions(+)
diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..5c1141175d51 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
#define ORC_TYPE_REGS_IRET 2
#define UNWIND_HINT_TYPE_SAVE 3
#define UNWIND_HINT_TYPE_RESTORE 4
+#define UNWIND_HINT_TYPE_RADDR_ALTER 6
+#define UNWIND_HINT_TYPE_RADDR_DELETE 7
#ifndef __ASSEMBLY__
/*
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..5211d2c5b7a2 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -94,6 +94,23 @@
UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
.endm
+/*
+ * RADDR_DELETE: Used on instructions (other than ret instruction)
+ * which remove a return address from the stack. count is the number
+ * of return address which are removed.
+ */
+.macro UNWIND_HINT_RADDR_DELETE count=1
+ UNWIND_HINT type=UNWIND_HINT_TYPE_RADDR_DELETE sp_offset=\count
+.endm
+
+/*
+ * RADDR_ALTER: Used on instructions which change a return address on
+ * the stack. count is the number of return address which are changed.
+ */
+.macro UNWIND_HINT_RADDR_ALTER count=1
+ UNWIND_HINT type=UNWIND_HINT_TYPE_RADDR_ALTER sp_offset=\count
+.endm
+
#else /* !__ASSEMBLY__ */
#define UNWIND_HINT(sp_reg, sp_offset, type, end) \
@@ -112,6 +129,12 @@
#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_RADDR_ALTER(count) \
+ UNWIND_HINT(0, count, UNWIND_HINT_TYPE_RADDR_ALTER, 0)
+
+#define UNWIND_HINT_RADDR_DELETE(count) \
+ UNWIND_HINT(0, count, UNWIND_HINT_TYPE_RADDR_DELETE, 0)
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_X86_UNWIND_HINTS_H */
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..5c1141175d51 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
#define ORC_TYPE_REGS_IRET 2
#define UNWIND_HINT_TYPE_SAVE 3
#define UNWIND_HINT_TYPE_RESTORE 4
+#define UNWIND_HINT_TYPE_RADDR_ALTER 6
+#define UNWIND_HINT_TYPE_RADDR_DELETE 7
#ifndef __ASSEMBLY__
/*
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8b1df659cd68..0574ce8e232d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -66,6 +66,28 @@ static bool raddr_pop(struct instruction **insn)
return true;
}
+static bool raddr_delete(int count)
+{
+ if (raddr_count < count)
+ return false;
+
+ raddr_count -= count;
+ return true;
+}
+
+static bool raddr_alter(int count)
+{
+ int i;
+
+ if (raddr_count < count)
+ false;
+
+ for (i = 0; i < count; i++)
+ raddr_list[raddr_count - 1 - i] = RADDR_ALTERED;
+
+ return true;
+}
+
static int validate_branch(struct objtool_file *file, struct symbol *func,
struct instruction *from,
struct instruction *first, struct insn_state state);
@@ -1314,6 +1336,14 @@ static int read_unwind_hints(struct objtool_file *file)
insn->restore = true;
insn->hint = true;
continue;
+
+ } else if (hint->type == UNWIND_HINT_TYPE_RADDR_DELETE) {
+ insn->raddr_delete = hint->sp_offset;
+ continue;
+
+ } else if (hint->type == UNWIND_HINT_TYPE_RADDR_ALTER) {
+ insn->raddr_alter = hint->sp_offset;
+ continue;
}
insn->hint = true;
@@ -1526,6 +1556,13 @@ static bool has_modified_stack_frame(struct insn_state *state)
state->drap)
return true;
+ /*
+ * If the stack was altered then don't check registers because
+ * a callee-saved registers might have been pushed on the stack.
+ */
+ if (state->stack_altered)
+ return false;
+
for (i = 0; i < CFI_NUM_REGS; i++)
if (state->regs[i].base != initial_func_cfi.regs[i].base ||
state->regs[i].offset != initial_func_cfi.regs[i].offset)
@@ -2103,6 +2140,15 @@ static int validate_return_address(struct objtool_file *file,
if (!raddr_insn)
break;
+ /*
+ * If this is a dynamic branch then we expect this
+ * branch to return or not, depending on the defined
+ * list of RA we have, so just continue the processing
+ * of the RA list.
+ */
+ if (raddr_insn == RADDR_ALTERED)
+ continue;
+
/*
* If we are branching to a defined address then
* just do an unconditional jump there.
@@ -2287,6 +2333,27 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 0;
}
+ if (insn->raddr_delete) {
+ /* delete return address */
+ if (!raddr_delete(insn->raddr_delete)) {
+ WARN_FUNC("fail to delete %d return address",
+ insn->sec, insn->offset,
+ insn->raddr_delete);
+ return 1;
+ }
+ state.stack_altered = true;
+
+ } else if (insn->raddr_alter) {
+ /* alter return address */
+ if (!raddr_alter(insn->raddr_alter)) {
+ WARN_FUNC("fail to alter %d return address",
+ insn->sec, insn->offset,
+ insn->raddr_alter);
+ return 1;
+ }
+ state.stack_altered = true;
+ }
+
switch (insn->type) {
case INSN_RETURN:
--
2.18.2
On 4/14/20 6:16 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
>> Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
>> hint macros to flag instructions which remove or modify return
>> addresses.
>
> I'm confused by this thing; why? AFAICT the rest of the patches are
> actually simpler without this one.
>
This is required to indicate to objtool that assembly instructions are
changing return addresses. For example, in patch 8:
For retpoline:
@@ -88,6 +96,7 @@
lfence
jmp .Lspec_trap_\@
.Ldo_rop_\@:
+ UNWIND_HINT_RADDR_ALTER
mov \reg, (%_ASM_SP)
ret
.endm
The unwind hint indicates that the return address has been altered, so the
code won't return to the return address which was on the stack.
Also for RSB stuffing, this is used to indicated that the stack adjustment
instruction (add $(BITS_PER_LONG/8) * 2, sp) is actually removing the last
two return addresses.
alex.
On 4/14/20 7:56 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 06:40:12PM +0200, Alexandre Chartre wrote:
>>
>> On 4/14/20 6:16 PM, Peter Zijlstra wrote:
>>> On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
>>>> Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
>>>> hint macros to flag instructions which remove or modify return
>>>> addresses.
>>>
>>> I'm confused by this thing; why? AFAICT the rest of the patches are
>>> actually simpler without this one.
>>>
>>
>> This is required to indicate to objtool that assembly instructions are
>> changing return addresses. For example, in patch 8:
>>
>> For retpoline:
>>
>> @@ -88,6 +96,7 @@
>> lfence
>> jmp .Lspec_trap_\@
>> .Ldo_rop_\@:
>> + UNWIND_HINT_RADDR_ALTER
>> mov \reg, (%_ASM_SP)
>> ret
>> .endm
>>
>> The unwind hint indicates that the return address has been altered, so the
>> code won't return to the return address which was on the stack.
>
> But if you hadn't added that return stack stuff in the first place,
> you'd not have needed that HINT.
>
> So what actual problem is it solving?
>
The return stack stuff is here to correctly handle intra-function call so that
we can figure out where the ret of an intra-function call should return. We
don't have this challenge with regular functions because we know that a ret
inside such function just indicates the end of the function.
But when there's an intra-function call, a ret instruction can either:
- continue after the intra-function call (if the stack was unchanged)
- jump somewhere else (if the return address was changed) and eventually
return to the next return address
- indicate the end of the function (if the return address was removed).
So, all this is needed to correctly follow the flow of the code and properly
record stack changes.
alex.
On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
> Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
> hint macros to flag instructions which remove or modify return
> addresses.
I'm confused by this thing; why? AFAICT the rest of the patches are
actually simpler without this one.
On Tue, Apr 14, 2020 at 06:40:12PM +0200, Alexandre Chartre wrote:
>
> On 4/14/20 6:16 PM, Peter Zijlstra wrote:
> > On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
> > > Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
> > > hint macros to flag instructions which remove or modify return
> > > addresses.
> >
> > I'm confused by this thing; why? AFAICT the rest of the patches are
> > actually simpler without this one.
> >
>
> This is required to indicate to objtool that assembly instructions are
> changing return addresses. For example, in patch 8:
>
> For retpoline:
>
> @@ -88,6 +96,7 @@
> lfence
> jmp .Lspec_trap_\@
> .Ldo_rop_\@:
> + UNWIND_HINT_RADDR_ALTER
> mov \reg, (%_ASM_SP)
> ret
> .endm
>
> The unwind hint indicates that the return address has been altered, so the
> code won't return to the return address which was on the stack.
But if you hadn't added that return stack stuff in the first place,
you'd not have needed that HINT.
So what actual problem is it solving?
On Tue, Apr 14, 2020 at 08:31:23PM +0200, Alexandre Chartre wrote:
> On 4/14/20 7:56 PM, Peter Zijlstra wrote:
> > So what actual problem is it solving?
> >
>
> The return stack stuff is here to correctly handle intra-function call so that
> we can figure out where the ret of an intra-function call should return. We
> don't have this challenge with regular functions because we know that a ret
> inside such function just indicates the end of the function.
>
> But when there's an intra-function call, a ret instruction can either:
> - continue after the intra-function call (if the stack was unchanged)
> - jump somewhere else (if the return address was changed) and eventually
> return to the next return address
> - indicate the end of the function (if the return address was removed).
>
> So, all this is needed to correctly follow the flow of the code and properly
> record stack changes.
But which intra-function calls are you worried about here? The RSB
stuffing ones we have to explicitly forget and the retpoline ones we
can't follow because they're indirect calls.
So again, who cares about that stack?
On Tue, Apr 14, 2020 at 09:27:27PM +0200, Alexandre Chartre wrote:
> This provides a generic code to handle any intra-function call. Currently we have
> the RSB stuffing ones which are forgotten with the UNWIND_HINT_TYPE_RADDR_DELETE
> directive. And for retpoline, they will not return if we have an indirect jump
> (JMP_NOSPEC) but they will return if we have an indirect call (CALL_NOSPEC). The
> code can handle both cases. For example, if we were to have a CALL_NOSPEC invocation
> which is not in an alternative then objtool can now correctly handle it.
The specialness of CALL_NOSPEC goes away with my proposed retpoline
rework as well. I really don't think we need something as complicated as
this.
Fundamentally validate_branch() will continue after a CALL instruction;
so I'm thikning the worst that can happen from not following a
(theoretical direct return) is a false-positive unreachable code
warning, and we can trivially fix those with exisiting hints.
On 4/14/20 8:42 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 08:31:23PM +0200, Alexandre Chartre wrote:
>> On 4/14/20 7:56 PM, Peter Zijlstra wrote:
>
>>> So what actual problem is it solving?
>>>
>>
>> The return stack stuff is here to correctly handle intra-function call so that
>> we can figure out where the ret of an intra-function call should return. We
>> don't have this challenge with regular functions because we know that a ret
>> inside such function just indicates the end of the function.
>>
>> But when there's an intra-function call, a ret instruction can either:
>> - continue after the intra-function call (if the stack was unchanged)
>> - jump somewhere else (if the return address was changed) and eventually
>> return to the next return address
>> - indicate the end of the function (if the return address was removed).
>>
>> So, all this is needed to correctly follow the flow of the code and properly
>> record stack changes.
>
> But which intra-function calls are you worried about here? The RSB
> stuffing ones we have to explicitly forget and the retpoline ones we
> can't follow because they're indirect calls.
>
> So again, who cares about that stack?
>
This provides a generic code to handle any intra-function call. Currently we have
the RSB stuffing ones which are forgotten with the UNWIND_HINT_TYPE_RADDR_DELETE
directive. And for retpoline, they will not return if we have an indirect jump
(JMP_NOSPEC) but they will return if we have an indirect call (CALL_NOSPEC). The
code can handle both cases. For example, if we were to have a CALL_NOSPEC invocation
which is not in an alternative then objtool can now correctly handle it.
alex.