Intra-function calls are implemented in objtool like unconditional
jumps. Keep track of intra-functin calls return addresses so that
objtool can make a return instruction continues the flow at the
right location.
Signed-off-by: Alexandre Chartre <[email protected]>
---
tools/objtool/arch/x86/decode.c | 7 +++
tools/objtool/check.c | 104 ++++++++++++++++++++++++++++++--
tools/objtool/check.h | 3 +
3 files changed, 110 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index f4d70b8835c4..76b593bb2e4f 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -427,6 +427,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
case 0xc2:
case 0xc3:
*type = INSN_RETURN;
+ /*
+ * For the impact on the stack, a ret behaves like
+ * a pop of the return address.
+ */
+ op->src.type = OP_SRC_POP;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_RA;
break;
case 0xca: /* retf */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index ad362c5de281..8b1df659cd68 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -26,9 +26,50 @@ struct alternative {
bool skip_orig;
};
+/*
+ * List to keep track of intra-function call return addresses.
+ * The list is a simple static array because we don't expect
+ * to have a lot of nested intra-function calls.
+ */
+#define RADDR_COUNT_MAX 32
+#define RADDR_ALTERED ((void *)-1)
+
+static struct instruction *raddr_list[RADDR_COUNT_MAX];
+static int raddr_count;
+
const char *objname;
struct cfi_state initial_func_cfi;
+static void raddr_clear(void)
+{
+ raddr_count = 0;
+}
+
+static bool raddr_push(struct instruction *insn)
+{
+ if (raddr_count == RADDR_COUNT_MAX) {
+ WARN_FUNC("return address list is full",
+ insn->sec, insn->offset);
+ return false;
+ }
+
+ raddr_list[raddr_count++] = insn;
+ return true;
+}
+
+static bool raddr_pop(struct instruction **insn)
+{
+ if (raddr_count == 0)
+ return false;
+
+ *insn = raddr_list[--raddr_count];
+ return true;
+}
+
+static int validate_branch(struct objtool_file *file, struct symbol *func,
+ struct instruction *from,
+ struct instruction *first, struct insn_state state);
+
struct instruction *find_insn(struct objtool_file *file,
struct section *sec, unsigned long offset)
{
@@ -2039,8 +2080,52 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
return validate_call(insn, state);
}
-static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
+static int validate_return_address(struct objtool_file *file,
+ struct symbol *func,
+ struct instruction *insn,
+ struct insn_state *state)
{
+ struct instruction *raddr_insn;
+ int ret;
+
+ while (raddr_pop(&raddr_insn)) {
+ /*
+ * We are branching somewhere and so processing
+ * a return instruction. So update the stack
+ * state for this instruction.
+ */
+ update_insn_state(insn, state);
+
+ /*
+ * If the return address has no instruction then
+ * that's the end of the function.
+ */
+ if (!raddr_insn)
+ break;
+
+ /*
+ * If we are branching to a defined address then
+ * just do an unconditional jump there.
+ */
+ ret = validate_branch(file, func, insn,
+ raddr_insn, *state);
+ if (ret) {
+ if (backtrace)
+ BT_FUNC("(ret-branch)", insn);
+ return ret;
+ }
+
+ return 0;
+ }
+
+ return -1;
+}
+
+static int validate_return(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state *state)
+{
+ int ret;
+
if (state->uaccess && !func_uaccess_safe(func)) {
WARN_FUNC("return with UACCESS enabled",
insn->sec, insn->offset);
@@ -2059,6 +2144,11 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
return 1;
}
+ /* check if we have return address to branch to */
+ ret = validate_return_address(file, func, insn, state);
+ if (ret >= 0)
+ return ret;
+
if (func && has_modified_stack_frame(state)) {
WARN_FUNC("return with modified stack frame",
insn->sec, insn->offset);
@@ -2200,7 +2290,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
switch (insn->type) {
case INSN_RETURN:
- return validate_return(func, insn, &state);
+ return validate_return(file, func, insn, &state);
case INSN_CALL:
case INSN_CALL_DYNAMIC:
@@ -2223,12 +2313,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
/*
* The call instruction can update the stack
* state. Then make the intra-function call
- * behaves like and unconditional jump.
+ * behaves like and unconditional jump. We
+ * track the return address to handle any
+ * return instruction.
*/
ret = update_insn_state(insn, &state);
if (ret)
return ret;
+ if (!raddr_push(next_insn))
+ return 1;
+
ret = validate_branch(file, func, insn,
insn->jump_dest, state);
if (ret) {
@@ -2383,6 +2478,7 @@ static int validate_unwind_hints(struct objtool_file *file)
for_each_insn(file, insn) {
if (insn->hint && !insn->visited) {
+ raddr_clear();
ret = validate_branch(file, insn->func,
NULL, insn, state);
if (ret && backtrace)
@@ -2522,7 +2618,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
continue;
state.uaccess = func->uaccess_safe;
-
+ raddr_clear();
ret = validate_branch(file, func, NULL, insn, state);
if (ret && backtrace)
BT_FUNC("<=== (func)", insn);
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6a80903fc4aa..f7dbecd46bed 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -23,6 +23,7 @@ struct insn_state {
unsigned int uaccess_stack;
int drap_reg, drap_offset;
struct cfi_reg vals[CFI_NUM_REGS];
+ bool stack_altered;
};
struct instruction {
@@ -39,6 +40,8 @@ struct instruction {
bool intra_function_call;
bool retpoline_safe;
u8 visited;
+ u8 raddr_delete;
+ u8 raddr_alter;
struct symbol *call_dest;
struct instruction *jump_dest;
struct instruction *first_jump_src;
--
2.18.2
Hi Alexandre,
On 4/14/20 11:36 AM, Alexandre Chartre wrote:
> Intra-function calls are implemented in objtool like unconditional
> jumps. Keep track of intra-functin calls return addresses so that
intra-function*
> objtool can make a return instruction continues the flow at the
> right location.
>
> Signed-off-by: Alexandre Chartre <[email protected]>
> ---
> tools/objtool/arch/x86/decode.c | 7 +++
> tools/objtool/check.c | 104 ++++++++++++++++++++++++++++++--
> tools/objtool/check.h | 3 +
> 3 files changed, 110 insertions(+), 4 deletions(-)
>
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index f4d70b8835c4..76b593bb2e4f 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -427,6 +427,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
> case 0xc2:
> case 0xc3:
> *type = INSN_RETURN;
> + /*
> + * For the impact on the stack, a ret behaves like
> + * a pop of the return address.
> + */
> + op->src.type = OP_SRC_POP;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_RA;
> break;
>
> case 0xca: /* retf */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index ad362c5de281..8b1df659cd68 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -26,9 +26,50 @@ struct alternative {
> bool skip_orig;
> };
>
> +/*
> + * List to keep track of intra-function call return addresses.
> + * The list is a simple static array because we don't expect
> + * to have a lot of nested intra-function calls.
> + */
> +#define RADDR_COUNT_MAX 32
> +#define RADDR_ALTERED ((void *)-1)
> +
> +static struct instruction *raddr_list[RADDR_COUNT_MAX];
> +static int raddr_count;
Could this be part of the insn_state?
> +
> const char *objname;
> struct cfi_state initial_func_cfi;
>
> +static void raddr_clear(void)
> +{
> + raddr_count = 0;
> +}
> +
> +static bool raddr_push(struct instruction *insn)
> +{
> + if (raddr_count == RADDR_COUNT_MAX) {
> + WARN_FUNC("return address list is full",
> + insn->sec, insn->offset);
> + return false;
> + }
> +
> + raddr_list[raddr_count++] = insn;
> + return true;
> +}
> +
> +static bool raddr_pop(struct instruction **insn)
> +{
> + if (raddr_count == 0)
> + return false;
> +
> + *insn = raddr_list[--raddr_count];
> + return true;
> +}
> +
> +static int validate_branch(struct objtool_file *file, struct symbol *func,
> + struct instruction *from,
> + struct instruction *first, struct insn_state state);
> +
> struct instruction *find_insn(struct objtool_file *file,
> struct section *sec, unsigned long offset)
> {
> @@ -2039,8 +2080,52 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
> return validate_call(insn, state);
> }
>
> -static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
> +static int validate_return_address(struct objtool_file *file,
> + struct symbol *func,
> + struct instruction *insn,
> + struct insn_state *state)
> {
> + struct instruction *raddr_insn;
> + int ret;
> +
> + while (raddr_pop(&raddr_insn)) {
Why is this a loop? Either there is something on the return address
stack, it gets validated as a branch and the function returns, or
nothing is on the stack and the function returns.
There doesn't seem to be a point where the loop gets to a second iteration.
> + /*
> + * We are branching somewhere and so processing
> + * a return instruction. So update the stack
> + * state for this instruction.
> + */
> + update_insn_state(insn, state);
> +
> + /*
> + * If the return address has no instruction then
> + * that's the end of the function.
> + */
> + if (!raddr_insn)
I can't think of a valid case where raddr_pop() returns "true" for a
NULL return address. If you check for that case, it should probably be a
warning/error rather than silently returning (especially since you'd
have updated the state).
> + break;
> +
> + /*
> + * If we are branching to a defined address then
> + * just do an unconditional jump there.
> + */
> + ret = validate_branch(file, func, insn,
> + raddr_insn, *state);
> + if (ret) {
> + if (backtrace)
> + BT_FUNC("(ret-branch)", insn);
> + return ret;
> + }
> +
> + return 0;
> + }
> +
> + return -1;
Returning "-1" is a bit confusing. One might think this is reporting an
error, but the only caller actually uses it as "I'll just carry on with
the rest of my checks".
Maybe validate_return() could call validate_return_address() as follows:
if (radd_pop(&raddr_insn))
return validate_return_address(file, func, insn, state, raddr_insn);
> +}
> +
> +static int validate_return(struct objtool_file *file, struct symbol *func,
> + struct instruction *insn, struct insn_state *state)
> +{
> + int ret;
> +
> if (state->uaccess && !func_uaccess_safe(func)) {
> WARN_FUNC("return with UACCESS enabled",
> insn->sec, insn->offset);
> @@ -2059,6 +2144,11 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
> return 1;
> }
>
> + /* check if we have return address to branch to */
> + ret = validate_return_address(file, func, insn, state);
> + if (ret >= 0)
> + return ret;
> +
> if (func && has_modified_stack_frame(state)) {
> WARN_FUNC("return with modified stack frame",
> insn->sec, insn->offset);
> @@ -2200,7 +2290,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> switch (insn->type) {
>
> case INSN_RETURN:
> - return validate_return(func, insn, &state);
> + return validate_return(file, func, insn, &state);
>
> case INSN_CALL:
> case INSN_CALL_DYNAMIC:
> @@ -2223,12 +2313,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> /*
> * The call instruction can update the stack
> * state. Then make the intra-function call
> - * behaves like and unconditional jump.
> + * behaves like and unconditional jump. We
> + * track the return address to handle any
> + * return instruction.
> */
> ret = update_insn_state(insn, &state);
> if (ret)
> return ret;
>
> + if (!raddr_push(next_insn))
> + return 1;
> +
> ret = validate_branch(file, func, insn,
> insn->jump_dest, state);
> if (ret) {
> @@ -2383,6 +2478,7 @@ static int validate_unwind_hints(struct objtool_file *file)
>
> for_each_insn(file, insn) {
> if (insn->hint && !insn->visited) {
> + raddr_clear();
> ret = validate_branch(file, insn->func,
> NULL, insn, state);
> if (ret && backtrace)
> @@ -2522,7 +2618,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
> continue;
>
> state.uaccess = func->uaccess_safe;
> -
> + raddr_clear();
> ret = validate_branch(file, func, NULL, insn, state);
> if (ret && backtrace)
> BT_FUNC("<=== (func)", insn);
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 6a80903fc4aa..f7dbecd46bed 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -23,6 +23,7 @@ struct insn_state {
> unsigned int uaccess_stack;
> int drap_reg, drap_offset;
> struct cfi_reg vals[CFI_NUM_REGS];
> + bool stack_altered;
This isn't used in this patch.
> };
>
> struct instruction {
> @@ -39,6 +40,8 @@ struct instruction {
> bool intra_function_call;
> bool retpoline_safe;
> u8 visited;
> + u8 raddr_delete;
> + u8 raddr_alter;
These fields (and the RADDR_ALTERED) don't seem to be used elsewhere in
this patch. I guess they should be part of patch 5.
> struct symbol *call_dest;
> struct instruction *jump_dest;
> struct instruction *first_jump_src;
>
Cheers,
--
Julien Thierry