Change objtool to support intra-function calls. An intra-function call
is represented in objtool as a push onto the stack (of the return
address), and a jump to the destination address. That way the stack
information is correctly updated and the call flow is still accurate.
Signed-off-by: Alexandre Chartre <[email protected]>
---
tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
tools/objtool/check.h | 1 +
2 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 214809ac2776..0cec91291d46 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
if (insn->type != INSN_CALL)
continue;
+ if (insn->intra_function_call) {
+ dest_off = insn->offset + insn->len + insn->immediate;
+ insn->jump_dest = find_insn(file, insn->sec, dest_off);
+ if (insn->jump_dest)
+ continue;
+
+ WARN_FUNC("can't find call dest at %s+0x%lx",
+ insn->sec, insn->offset,
+ insn->sec->name, dest_off);
+ return -1;
+ }
+
rela = find_rela_by_dest_range(insn->sec, insn->offset,
insn->len);
if (!rela) {
@@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
return 0;
}
+static int read_intra_function_call(struct objtool_file *file)
+{
+ struct section *sec;
+ struct instruction *insn;
+ struct rela *rela;
+
+ sec = find_section_by_name(file->elf,
+ ".rela.discard.intra_function_call");
+ if (!sec)
+ return 0;
+
+ list_for_each_entry(rela, &sec->rela_list, list) {
+ if (rela->sym->type != STT_SECTION) {
+ WARN("unexpected relocation symbol type in %s",
+ sec->name);
+ return -1;
+ }
+
+ insn = find_insn(file, rela->sym->sec, rela->addend);
+ if (!insn) {
+ WARN("bad .discard.intra_function_call entry");
+ return -1;
+ }
+
+ if (insn->type != INSN_CALL) {
+ WARN_FUNC("intra_function_call not a call",
+ insn->sec, insn->offset);
+ return -1;
+ }
+
+ insn->intra_function_call = true;
+ /*
+ * For the impact on the stack, make an intra-function
+ * call behaves like a push of an immediate value (the
+ * return address).
+ */
+ insn->stack_op.src.type = OP_SRC_CONST;
+ insn->stack_op.dest.type = OP_DEST_PUSH;
+ }
+
+ return 0;
+}
+
static void mark_rodata(struct objtool_file *file)
{
struct section *sec;
@@ -1344,6 +1399,10 @@ static int decode_sections(struct objtool_file *file)
if (ret)
return ret;
+ ret = read_intra_function_call(file);
+ if (ret)
+ return ret;
+
ret = add_call_destinations(file);
if (ret)
return ret;
@@ -2092,7 +2151,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return ret;
if (!no_fp && func && !is_fentry_call(insn) &&
- !has_valid_stack_frame(&state)) {
+ !has_valid_stack_frame(&state) &&
+ !insn->intra_function_call) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
return 1;
@@ -2101,6 +2161,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (dead_end_function(file, insn->call_dest))
return 0;
+ if (insn->intra_function_call) {
+ update_insn_state(insn, &state);
+ ret = validate_branch(file, func, insn,
+ insn->jump_dest, state);
+ if (ret) {
+ if (backtrace)
+ BT_FUNC("(intra-call)", insn);
+ return ret;
+ }
+ }
+
break;
case INSN_JUMP_CONDITIONAL:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index cffb23d81782..2bd6d2f46baa 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -35,6 +35,7 @@ struct instruction {
unsigned long immediate;
unsigned int alt_group;
bool dead_end, ignore, hint, save, restore, ignore_alts;
+ bool intra_function_call;
bool retpoline_safe;
u8 visited;
struct symbol *call_dest;
--
2.18.2
Hi Alexandre,
I ran into the limitation of intra-function call for the arm64 support
but didn't take the time to make a clean patch to support them properly.
Nice to see you've gone through that work :) .
On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> Change objtool to support intra-function calls. An intra-function call
> is represented in objtool as a push onto the stack (of the return
I have to disagree a bit with that. The push onto the stack is true on
x86, but other architectures might not have that (arm/arm64 have a link
register that gets set by "bl" instructions and do not modify the stack).
> address), and a jump to the destination address. That way the stack
> information is correctly updated and the call flow is still accurate.
>
> Signed-off-by: Alexandre Chartre <[email protected]>
> ---
> tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
> tools/objtool/check.h | 1 +
> 2 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 214809ac2776..0cec91291d46 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
> if (insn->type != INSN_CALL)
> continue;
>
> + if (insn->intra_function_call) {
> + dest_off = insn->offset + insn->len + insn->immediate;
> + insn->jump_dest = find_insn(file, insn->sec, dest_off);
> + if (insn->jump_dest)
> + continue;
> +
> + WARN_FUNC("can't find call dest at %s+0x%lx",
> + insn->sec, insn->offset,
> + insn->sec->name, dest_off);
> + return -1;
> + }
> +
> rela = find_rela_by_dest_range(insn->sec, insn->offset,
> insn->len);
> if (!rela) {
> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
> return 0;
> }
>
> +static int read_intra_function_call(struct objtool_file *file)
> +{
> + struct section *sec;
> + struct instruction *insn;
> + struct rela *rela;
> +
> + sec = find_section_by_name(file->elf,
> + ".rela.discard.intra_function_call");
I'm wondering, do we really need to annotate the intra_function_call and
group the in a section?
Would it be a problem to consider all (static) call instructions with a
destination that is not the start offset of a symbol to be an
intra-function call (and set insn->intra_function_call and
insn->jump_dest accordingly)?
Other than that the logic would stay the same.
> + if (!sec)
> + return 0;
> +
> + list_for_each_entry(rela, &sec->rela_list, list) {
> + if (rela->sym->type != STT_SECTION) {
> + WARN("unexpected relocation symbol type in %s",
> + sec->name);
> + return -1;
> + }
> +
> + insn = find_insn(file, rela->sym->sec, rela->addend);
> + if (!insn) {
> + WARN("bad .discard.intra_function_call entry");
> + return -1;
> + }
> +
> + if (insn->type != INSN_CALL) {
> + WARN_FUNC("intra_function_call not a call",
> + insn->sec, insn->offset);
Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
"unsupported instruction for intra-function call " ?
> + return -1;
> + }
> +
> + insn->intra_function_call = true;
> + /*
> + * For the impact on the stack, make an intra-function
> + * call behaves like a push of an immediate value (the
> + * return address).
> + */
> + insn->stack_op.src.type = OP_SRC_CONST;
> + insn->stack_op.dest.type = OP_DEST_PUSH;
As commented above, this should be arch dependent.
> + }
> +
> + return 0;
> +}
> +
> static void mark_rodata(struct objtool_file *file)
> {
> struct section *sec;
> @@ -1344,6 +1399,10 @@ static int decode_sections(struct objtool_file *file)
> if (ret)
> return ret;
>
> + ret = read_intra_function_call(file);
> + if (ret)
> + return ret;
> +
> ret = add_call_destinations(file);
> if (ret)
> return ret;
> @@ -2092,7 +2151,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> return ret;
>
> if (!no_fp && func && !is_fentry_call(insn) &&
> - !has_valid_stack_frame(&state)) {
> + !has_valid_stack_frame(&state) &&
> + !insn->intra_function_call) {
> WARN_FUNC("call without frame pointer save/setup",
> sec, insn->offset);
> return 1;
> @@ -2101,6 +2161,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> if (dead_end_function(file, insn->call_dest))
> return 0;
>
> + if (insn->intra_function_call) {
> + update_insn_state(insn, &state);
> + ret = validate_branch(file, func, insn,
> + insn->jump_dest, state);
> + if (ret) {
> + if (backtrace)
> + BT_FUNC("(intra-call)", insn);
> + return ret;
> + }
> + }
> +
> break;
>
> case INSN_JUMP_CONDITIONAL:
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index cffb23d81782..2bd6d2f46baa 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -35,6 +35,7 @@ struct instruction {
> unsigned long immediate;
> unsigned int alt_group;
> bool dead_end, ignore, hint, save, restore, ignore_alts;
> + bool intra_function_call;
> bool retpoline_safe;
> u8 visited;
> struct symbol *call_dest;
>
Thanks,
--
Julien Thierry
On 4/2/20 2:53 PM, Julien Thierry wrote:
> Hi Alexandre,
>
> I ran into the limitation of intra-function call for the arm64
> support but didn't take the time to make a clean patch to support
> them properly.
>
> Nice to see you've gone through that work :) .
>
> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>> Change objtool to support intra-function calls. An intra-function call
>> is represented in objtool as a push onto the stack (of the return
>
> I have to disagree a bit with that. The push onto the stack is true
> on x86, but other architectures might not have that (arm/arm64 have a
> link register that gets set by "bl" instructions and do not modify
> the stack).
Correct, this is x86 specific.
>
>> address), and a jump to the destination address. That way the stack
>> information is correctly updated and the call flow is still accurate.
>>
>> Signed-off-by: Alexandre Chartre <[email protected]>
>> ---
>> ? tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
>> ? tools/objtool/check.h |? 1 +
>> ? 2 files changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 214809ac2776..0cec91291d46 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
>> ????????? if (insn->type != INSN_CALL)
>> ????????????? continue;
>> +??????? if (insn->intra_function_call) {
>> +??????????? dest_off = insn->offset + insn->len + insn->immediate;
>> +??????????? insn->jump_dest = find_insn(file, insn->sec, dest_off);
>> +??????????? if (insn->jump_dest)
>> +??????????????? continue;
>> +
>> +??????????? WARN_FUNC("can't find call dest at %s+0x%lx",
>> +????????????????? insn->sec, insn->offset,
>> +????????????????? insn->sec->name, dest_off);
>> +??????????? return -1;
>> +??????? }
>> +
>> ????????? rela = find_rela_by_dest_range(insn->sec, insn->offset,
>> ???????????????????????????? insn->len);
>> ????????? if (!rela) {
>> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
>> ????? return 0;
>> ? }
>> +static int read_intra_function_call(struct objtool_file *file)
>> +{
>> +??? struct section *sec;
>> +??? struct instruction *insn;
>> +??? struct rela *rela;
>> +
>> +??? sec = find_section_by_name(file->elf,
>> +?????????????????? ".rela.discard.intra_function_call");
>
> I'm wondering, do we really need to annotate the intra_function_call
> and group the in a section?
>
> Would it be a problem to consider all (static) call instructions with
> a destination that is not the start offset of a symbol to be an
> intra-function call (and set insn->intra_function_call and
> insn->jump_dest accordingly)?
Correct, we could automatically detect intra-function calls instead of
having to annotate them. However, I choose to annotate them because I don't
think that's not an expected construct in a "normal" code flow (at least
on x86). So objtool would still issue a warning on intra-function calls
by default, and you can annotate them to indicate if they are expected.
If intra-function calls are frequent on arm then I can add an option to
objtool so it automatically detects them. This way, we won't use the option
on x86 and we have to annotate intra-function call on x86, and you can
use it on arm to automatically detect intra-function calls.
> Other than that the logic would stay the same.
>
>> +??? if (!sec)
>> +??????? return 0;
>> +
>> +??? list_for_each_entry(rela, &sec->rela_list, list) {
>> +??????? if (rela->sym->type != STT_SECTION) {
>> +??????????? WARN("unexpected relocation symbol type in %s",
>> +???????????????? sec->name);
>> +??????????? return -1;
>> +??????? }
>> +
>> +??????? insn = find_insn(file, rela->sym->sec, rela->addend);
>> +??????? if (!insn) {
>> +??????????? WARN("bad .discard.intra_function_call entry");
>> +??????????? return -1;
>> +??????? }
>> +
>> +??????? if (insn->type != INSN_CALL) {
>> +??????????? WARN_FUNC("intra_function_call not a call",
>> +????????????????? insn->sec, insn->offset);
>
> Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
> ????"unsupported instruction for intra-function call " ?
Right, I will change that: "intra_function_call not a direct call"
>> +??????????? return -1;
>> +??????? }
>> +
>> +??????? insn->intra_function_call = true;
>> +??????? /*
>> +???????? * For the impact on the stack, make an intra-function
>> +???????? * call behaves like a push of an immediate value (the
>> +???????? * return address).
>> +???????? */
>> +??????? insn->stack_op.src.type = OP_SRC_CONST;
>> +??????? insn->stack_op.dest.type = OP_DEST_PUSH;
>
> As commented above, this should be arch dependent.
I will add a arch dependent call. I will also do that for the return
trampoline call case (patch 4).
Thanks,
alex.
>> +??? }
>> +
>> +??? return 0;
>> +}
>> +
>> ? static void mark_rodata(struct objtool_file *file)
>> ? {
>> ????? struct section *sec;
>> @@ -1344,6 +1399,10 @@ static int decode_sections(struct objtool_file *file)
>> ????? if (ret)
>> ????????? return ret;
>> +??? ret = read_intra_function_call(file);
>> +??? if (ret)
>> +??????? return ret;
>> +
>> ????? ret = add_call_destinations(file);
>> ????? if (ret)
>> ????????? return ret;
>> @@ -2092,7 +2151,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>> ????????????????? return ret;
>> ????????????? if (!no_fp && func && !is_fentry_call(insn) &&
>> -??????????????? !has_valid_stack_frame(&state)) {
>> +??????????????? !has_valid_stack_frame(&state) &&
>> +??????????????? !insn->intra_function_call) {
>> ????????????????? WARN_FUNC("call without frame pointer save/setup",
>> ??????????????????????? sec, insn->offset);
>> ????????????????? return 1;
>> @@ -2101,6 +2161,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>> ????????????? if (dead_end_function(file, insn->call_dest))
>> ????????????????? return 0;
>> +??????????? if (insn->intra_function_call) {
>> +??????????????? update_insn_state(insn, &state);
>> +??????????????? ret = validate_branch(file, func, insn,
>> +????????????????????????????? insn->jump_dest, state);
>> +??????????????? if (ret) {
>> +??????????????????? if (backtrace)
>> +??????????????????????? BT_FUNC("(intra-call)", insn);
>> +??????????????????? return ret;
>> +??????????????? }
>> +??????????? }
>> +
>> ????????????? break;
>> ????????? case INSN_JUMP_CONDITIONAL:
>> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
>> index cffb23d81782..2bd6d2f46baa 100644
>> --- a/tools/objtool/check.h
>> +++ b/tools/objtool/check.h
>> @@ -35,6 +35,7 @@ struct instruction {
>> ????? unsigned long immediate;
>> ????? unsigned int alt_group;
>> ????? bool dead_end, ignore, hint, save, restore, ignore_alts;
>> +??? bool intra_function_call;
>> ????? bool retpoline_safe;
>> ????? u8 visited;
>> ????? struct symbol *call_dest;
>>
>
> Thanks,
>
On 4/2/20 2:24 PM, Alexandre Chartre wrote:
>
>
> On 4/2/20 2:53 PM, Julien Thierry wrote:
>> Hi Alexandre,
>>
>> I ran into the limitation of intra-function call for the arm64
>> support but didn't take the time to make a clean patch to support
>> them properly.
>>
>> Nice to see you've gone through that work :) .
>>
>> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>>> Change objtool to support intra-function calls. An intra-function call
>>> is represented in objtool as a push onto the stack (of the return
>>
>> I have to disagree a bit with that. The push onto the stack is true
>> on x86, but other architectures might not have that (arm/arm64 have a
>> link register that gets set by "bl" instructions and do not modify
>> the stack).
>
> Correct, this is x86 specific.
>
>>
>>> address), and a jump to the destination address. That way the stack
>>> information is correctly updated and the call flow is still accurate.
>>>
>>> Signed-off-by: Alexandre Chartre <[email protected]>
>>> ---
>>> ? tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
>>> ? tools/objtool/check.h |? 1 +
>>> ? 2 files changed, 73 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>> index 214809ac2776..0cec91291d46 100644
>>> --- a/tools/objtool/check.c
>>> +++ b/tools/objtool/check.c
>>> @@ -657,6 +657,18 @@ static int add_call_destinations(struct
>>> objtool_file *file)
>>> ????????? if (insn->type != INSN_CALL)
>>> ????????????? continue;
>>> +??????? if (insn->intra_function_call) {
>>> +??????????? dest_off = insn->offset + insn->len + insn->immediate;
>>> +??????????? insn->jump_dest = find_insn(file, insn->sec, dest_off);
>>> +??????????? if (insn->jump_dest)
>>> +??????????????? continue;
>>> +
>>> +??????????? WARN_FUNC("can't find call dest at %s+0x%lx",
>>> +????????????????? insn->sec, insn->offset,
>>> +????????????????? insn->sec->name, dest_off);
>>> +??????????? return -1;
>>> +??????? }
>>> +
>>> ????????? rela = find_rela_by_dest_range(insn->sec, insn->offset,
>>> ???????????????????????????? insn->len);
>>> ????????? if (!rela) {
>>> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct
>>> objtool_file *file)
>>> ????? return 0;
>>> ? }
>>> +static int read_intra_function_call(struct objtool_file *file)
>>> +{
>>> +??? struct section *sec;
>>> +??? struct instruction *insn;
>>> +??? struct rela *rela;
>>> +
>>> +??? sec = find_section_by_name(file->elf,
>>> +?????????????????? ".rela.discard.intra_function_call");
>>
>> I'm wondering, do we really need to annotate the intra_function_call
>> and group the in a section?
>>
>> Would it be a problem to consider all (static) call instructions with
>> a destination that is not the start offset of a symbol to be an
>> intra-function call (and set insn->intra_function_call and
>> insn->jump_dest accordingly)?
>
> Correct, we could automatically detect intra-function calls instead of
> having to annotate them. However, I choose to annotate them because I don't
> think that's not an expected construct in a "normal" code flow (at least
> on x86). So objtool would still issue a warning on intra-function calls
> by default, and you can annotate them to indicate if they are expected.
>
> If intra-function calls are frequent on arm then I can add an option to
> objtool so it automatically detects them. This way, we won't use the option
> on x86 and we have to annotate intra-function call on x86, and you can
> use it on arm to automatically detect intra-function calls.
>
That makes sense. Maybe we can just allow them in !file->c_file, I don't
think gcc generates such call on arm64, so I think we'd only have that
in assembly.
If people prefer to keep the annotation, would you mind having a
"ANNOTATE_INTRA_FUNCTION_CALL" macro in include/linux/frame.h to add the
label and the reference to the right section?
This way it could be reused for other archs.
>
>> Other than that the logic would stay the same.
>>
>>> +??? if (!sec)
>>> +??????? return 0;
>>> +
>>> +??? list_for_each_entry(rela, &sec->rela_list, list) {
>>> +??????? if (rela->sym->type != STT_SECTION) {
>>> +??????????? WARN("unexpected relocation symbol type in %s",
>>> +???????????????? sec->name);
>>> +??????????? return -1;
>>> +??????? }
>>> +
>>> +??????? insn = find_insn(file, rela->sym->sec, rela->addend);
>>> +??????? if (!insn) {
>>> +??????????? WARN("bad .discard.intra_function_call entry");
>>> +??????????? return -1;
>>> +??????? }
>>> +
>>> +??????? if (insn->type != INSN_CALL) {
>>> +??????????? WARN_FUNC("intra_function_call not a call",
>>> +????????????????? insn->sec, insn->offset);
>>
>> Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
>> ?????"unsupported instruction for intra-function call " ?
>
> Right, I will change that: "intra_function_call not a direct call"
>
>>> +??????????? return -1;
>>> +??????? }
>>> +
>>> +??????? insn->intra_function_call = true;
>>> +??????? /*
>>> +???????? * For the impact on the stack, make an intra-function
>>> +???????? * call behaves like a push of an immediate value (the
>>> +???????? * return address).
>>> +???????? */
>>> +??????? insn->stack_op.src.type = OP_SRC_CONST;
>>> +??????? insn->stack_op.dest.type = OP_DEST_PUSH;
>>
>> As commented above, this should be arch dependent.
>
> I will add a arch dependent call. I will also do that for the return
> trampoline call case (patch 4).
>
Thank you!
--
Julien Thierry
On Thu, Apr 02, 2020 at 03:24:45PM +0200, Alexandre Chartre wrote:
> On 4/2/20 2:53 PM, Julien Thierry wrote:
> > On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> > > +??? sec = find_section_by_name(file->elf,
> > > +?????????????????? ".rela.discard.intra_function_call");
> >
> > I'm wondering, do we really need to annotate the intra_function_call
> > and group the in a section?
> >
> > Would it be a problem to consider all (static) call instructions with
> > a destination that is not the start offset of a symbol to be an
> > intra-function call (and set insn->intra_function_call and
> > insn->jump_dest accordingly)?
>
> Correct, we could automatically detect intra-function calls instead of
> having to annotate them. However, I choose to annotate them because I don't
> think that's not an expected construct in a "normal" code flow (at least
> on x86). So objtool would still issue a warning on intra-function calls
> by default, and you can annotate them to indicate if they are expected.
I wondered the same thing when reading the patch. I'm confliected on
this. On the one hand auto-detecting this seems like an excellent idea.
If/when the compiler generates them, they had better be okay too.
Josh?
On Thu, Apr 02, 2020 at 05:04:07PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 03:24:45PM +0200, Alexandre Chartre wrote:
> > On 4/2/20 2:53 PM, Julien Thierry wrote:
> > > On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>
> > > > +Â Â Â sec = find_section_by_name(file->elf,
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ".rela.discard.intra_function_call");
> > >
> > > I'm wondering, do we really need to annotate the intra_function_call
> > > and group the in a section?
> > >
> > > Would it be a problem to consider all (static) call instructions with
> > > a destination that is not the start offset of a symbol to be an
> > > intra-function call (and set insn->intra_function_call and
> > > insn->jump_dest accordingly)?
> >
> > Correct, we could automatically detect intra-function calls instead of
> > having to annotate them. However, I choose to annotate them because I don't
> > think that's not an expected construct in a "normal" code flow (at least
> > on x86). So objtool would still issue a warning on intra-function calls
> > by default, and you can annotate them to indicate if they are expected.
>
> I wondered the same thing when reading the patch. I'm confliected on
> this. On the one hand auto-detecting this seems like an excellent idea.
>
> If/when the compiler generates them, they had better be okay too.
>
> Josh?
In general I prefer to keep it simple, and keep the annotations to a
minimum. And I don't think this warning has ever found anything useful.
So I'd be inclined to say just allow them and automatically detect them.
However the fact that arm64 asm actually uses them worries me a bit.
So for me it kind of hinges on whether arm64 has a legitimate use case
for them, or if the warning actually points to smelly code.
--
Josh
On 4/2/20 3:38 PM, Julien Thierry wrote:
>
>
> On 4/2/20 2:24 PM, Alexandre Chartre wrote:
>>
>>
>> On 4/2/20 2:53 PM, Julien Thierry wrote:
>>> Hi Alexandre,
>>>
>>> I ran into the limitation of intra-function call for the arm64
>>> support but didn't take the time to make a clean patch to support
>>> them properly.
>>>
>>> Nice to see you've gone through that work :) .
>>>
>>> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>>>> Change objtool to support intra-function calls. An intra-function call
>>>> is represented in objtool as a push onto the stack (of the return
>>>
>>> I have to disagree a bit with that. The push onto the stack is true
>>> on x86, but other architectures might not have that (arm/arm64 have a
>>> link register that gets set by "bl" instructions and do not modify
>>> the stack).
>>
>> Correct, this is x86 specific.
>>
>>>
>>>> address), and a jump to the destination address. That way the stack
>>>> information is correctly updated and the call flow is still accurate.
>>>>
>>>> Signed-off-by: Alexandre Chartre <[email protected]>
>>>> ---
>>>> ? tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
>>>> ? tools/objtool/check.h |? 1 +
>>>> ? 2 files changed, 73 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>>> index 214809ac2776..0cec91291d46 100644
>>>> --- a/tools/objtool/check.c
>>>> +++ b/tools/objtool/check.c
>>>> @@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
>>>> ????????? if (insn->type != INSN_CALL)
>>>> ????????????? continue;
>>>> +??????? if (insn->intra_function_call) {
>>>> +??????????? dest_off = insn->offset + insn->len + insn->immediate;
>>>> +??????????? insn->jump_dest = find_insn(file, insn->sec, dest_off);
>>>> +??????????? if (insn->jump_dest)
>>>> +??????????????? continue;
>>>> +
>>>> +??????????? WARN_FUNC("can't find call dest at %s+0x%lx",
>>>> +????????????????? insn->sec, insn->offset,
>>>> +????????????????? insn->sec->name, dest_off);
>>>> +??????????? return -1;
>>>> +??????? }
>>>> +
>>>> ????????? rela = find_rela_by_dest_range(insn->sec, insn->offset,
>>>> ???????????????????????????? insn->len);
>>>> ????????? if (!rela) {
>>>> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
>>>> ????? return 0;
>>>> ? }
>>>> +static int read_intra_function_call(struct objtool_file *file)
>>>> +{
>>>> +??? struct section *sec;
>>>> +??? struct instruction *insn;
>>>> +??? struct rela *rela;
>>>> +
>>>> +??? sec = find_section_by_name(file->elf,
>>>> +?????????????????? ".rela.discard.intra_function_call");
>>>
>>> I'm wondering, do we really need to annotate the intra_function_call
>>> and group the in a section?
>>>
>>> Would it be a problem to consider all (static) call instructions with
>>> a destination that is not the start offset of a symbol to be an
>>> intra-function call (and set insn->intra_function_call and
>>> insn->jump_dest accordingly)?
>>
>> Correct, we could automatically detect intra-function calls instead of
>> having to annotate them. However, I choose to annotate them because I don't
>> think that's not an expected construct in a "normal" code flow (at least
>> on x86). So objtool would still issue a warning on intra-function calls
>> by default, and you can annotate them to indicate if they are expected.
>>
>> If intra-function calls are frequent on arm then I can add an option to
>> objtool so it automatically detects them. This way, we won't use the option
>> on x86 and we have to annotate intra-function call on x86, and you can
>> use it on arm to automatically detect intra-function calls.
>>
>
> That makes sense. Maybe we can just allow them in !file->c_file, I
> don't think gcc generates such call on arm64, so I think we'd only
> have that in assembly.
We can have also intra-function call in C file with the asm directive, for
example with retpoline. Actually I think I forgot to check that as this is
only on 32bit on x86.
> If people prefer to keep the annotation, would you mind having a
> "ANNOTATE_INTRA_FUNCTION_CALL" macro in include/linux/frame.h to add
> the label and the reference to the right section?
>
> This way it could be reused for other archs.
Sure, I will do that.
alex.
>>
>>> Other than that the logic would stay the same.
>>>
>>>> +??? if (!sec)
>>>> +??????? return 0;
>>>> +
>>>> +??? list_for_each_entry(rela, &sec->rela_list, list) {
>>>> +??????? if (rela->sym->type != STT_SECTION) {
>>>> +??????????? WARN("unexpected relocation symbol type in %s",
>>>> +???????????????? sec->name);
>>>> +??????????? return -1;
>>>> +??????? }
>>>> +
>>>> +??????? insn = find_insn(file, rela->sym->sec, rela->addend);
>>>> +??????? if (!insn) {
>>>> +??????????? WARN("bad .discard.intra_function_call entry");
>>>> +??????????? return -1;
>>>> +??????? }
>>>> +
>>>> +??????? if (insn->type != INSN_CALL) {
>>>> +??????????? WARN_FUNC("intra_function_call not a call",
>>>> +????????????????? insn->sec, insn->offset);
>>>
>>> Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
>>> ?????"unsupported instruction for intra-function call " ?
>>
>> Right, I will change that: "intra_function_call not a direct call"
>>
>>>> +??????????? return -1;
>>>> +??????? }
>>>> +
>>>> +??????? insn->intra_function_call = true;
>>>> +??????? /*
>>>> +???????? * For the impact on the stack, make an intra-function
>>>> +???????? * call behaves like a push of an immediate value (the
>>>> +???????? * return address).
>>>> +???????? */
>>>> +??????? insn->stack_op.src.type = OP_SRC_CONST;
>>>> +??????? insn->stack_op.dest.type = OP_DEST_PUSH;
>>>
>>> As commented above, this should be arch dependent.
>>
>> I will add a arch dependent call. I will also do that for the return
>> trampoline call case (patch 4).
>>
>
> Thank you!
>
On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
> Hi Alexandre,
>
> I ran into the limitation of intra-function call for the arm64 support but
> didn't take the time to make a clean patch to support them properly.
Can you give an example of where arm64 uses intra-function calls? It
sounds sketchy to me :-) Is it really needed/useful?
--
Josh
On Thu, Apr 02, 2020 at 10:49:19AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
> > Hi Alexandre,
> >
> > I ran into the limitation of intra-function call for the arm64 support but
> > didn't take the time to make a clean patch to support them properly.
>
> Can you give an example of where arm64 uses intra-function calls? It
> sounds sketchy to me :-) Is it really needed/useful?
BTW, I have a vague memory of discussing this before, so I apologize if
I'm repeating myself ;-)
--
Josh
On 4/2/20 5:54 PM, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 05:04:07PM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 02, 2020 at 03:24:45PM +0200, Alexandre Chartre wrote:
>>> On 4/2/20 2:53 PM, Julien Thierry wrote:
>>>> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>>
>>>>> +Â Â Â sec = find_section_by_name(file->elf,
>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ".rela.discard.intra_function_call");
>>>>
>>>> I'm wondering, do we really need to annotate the intra_function_call
>>>> and group the in a section?
>>>>
>>>> Would it be a problem to consider all (static) call instructions with
>>>> a destination that is not the start offset of a symbol to be an
>>>> intra-function call (and set insn->intra_function_call and
>>>> insn->jump_dest accordingly)?
>>>
>>> Correct, we could automatically detect intra-function calls instead of
>>> having to annotate them. However, I choose to annotate them because I don't
>>> think that's not an expected construct in a "normal" code flow (at least
>>> on x86). So objtool would still issue a warning on intra-function calls
>>> by default, and you can annotate them to indicate if they are expected.
>>
>> I wondered the same thing when reading the patch. I'm confliected on
>> this. On the one hand auto-detecting this seems like an excellent idea.
>>
>> If/when the compiler generates them, they had better be okay too.
>>
>> Josh?
>
> In general I prefer to keep it simple, and keep the annotations to a
> minimum. And I don't think this warning has ever found anything useful.
> So I'd be inclined to say just allow them and automatically detect them.
>
> However the fact that arm64 asm actually uses them worries me a bit.
>
> So for me it kind of hinges on whether arm64 has a legitimate use case
> for them, or if the warning actually points to smelly code.
>
Then what I can do is:
- by default, automatically detect and validate intra-function calls
- remove the INTRA_FUNCTION_CALL annotation
- add an objtool option to print a warning about intra-function calls
(but still validate such calls).
This way by default intra-function calls are automatically detected and
validated. But you still have the option to print them out if you want
to check if there are any intra-function calls.
alex.
On 4/2/20 4:49 PM, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
>> Hi Alexandre,
>>
>> I ran into the limitation of intra-function call for the arm64 support but
>> didn't take the time to make a clean patch to support them properly.
>
> Can you give an example of where arm64 uses intra-function calls? It
> sounds sketchy to me :-) Is it really needed/useful?
>
So the most notable/necessary one(s) is the one in tramp_ventry [1].
This macro is used as the begining of exception handlers for exceptions
coming from userland. It was added as part of the mitigations of spectre
(v1???).
To give some context, x30 is the register that "ret" instruction will
use as return address, "bl" is the equivalent of x86 "call" and sets x30
before jumping to the target address. (However, it doesn't have a
special semantic for exception returns)
Note: I believe the comment about the return "stack" is about processor
internal state (speculative thingies and all) rather than the actual
stack, since the stack is untouched by that code. But I don't know the
actual details.
There are also some in arch/arm64/crypto/crct10dif-ce-core.o , which is
probably full of fast, smart and optimized code I don't understand :) .
So I wouldn't feel confident commenting on whether those intra-function
calls are needed or not.
Last I found is in qcom_link_stack_sanitization() [2], but that's just a
workaround for a very specific hardware. In my local tree I just put the
function as STACK_FRAME_NON_STANDARD. But the code just saves the return
address, has 16 call instructions that just call the instruction after
them, restores the return address and lets the C-function return
normally (and it somehow fixes something for that hardware).
Those are the ones I stumbled on. So yes, it a bit sketchy, corner case
code, but it's there and unlikely to go away.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/entry.S?h=v5.6#n803
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/cpu_errata.c?h=v5.6#n195
Cheers,
--
Julien Thierry
On 4/3/20 1:41 PM, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
>>
>> Last I found is in qcom_link_stack_sanitization() [2], but that's just a
>> workaround for a very specific hardware. In my local tree I just put the
>> function as STACK_FRAME_NON_STANDARD. But the code just saves the return
>> address, has 16 call instructions that just call the instruction after them,
>> restores the return address and lets the C-function return normally (and it
>> somehow fixes something for that hardware).
>>
> That sounds very much like the RSB flushing we do.
>
Yes, the piece of code you posted reminded me of this. The difference is
that the RSB part uses a loop and counter while the qcom thing has a
fixed amount of call instructions (which can make things easier for
static analysis, if we'd really want to go down that road).
--
Julien Thierry
On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
>
> Last I found is in qcom_link_stack_sanitization() [2], but that's just a
> workaround for a very specific hardware. In my local tree I just put the
> function as STACK_FRAME_NON_STANDARD. But the code just saves the return
> address, has 16 call instructions that just call the instruction after them,
> restores the return address and lets the C-function return normally (and it
> somehow fixes something for that hardware).
>
That sounds very much like the RSB flushing we do.
On Fri, Apr 03, 2020 at 01:49:24PM +0100, Julien Thierry wrote:
>
>
> On 4/3/20 1:41 PM, Peter Zijlstra wrote:
> > On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
> > >
> > > Last I found is in qcom_link_stack_sanitization() [2], but that's just a
> > > workaround for a very specific hardware. In my local tree I just put the
> > > function as STACK_FRAME_NON_STANDARD. But the code just saves the return
> > > address, has 16 call instructions that just call the instruction after them,
> > > restores the return address and lets the C-function return normally (and it
> > > somehow fixes something for that hardware).
> > >
> > That sounds very much like the RSB flushing we do.
> >
>
> Yes, the piece of code you posted reminded me of this. The difference is
> that the RSB part uses a loop and counter while the qcom thing has a fixed
> amount of call instructions (which can make things easier for static
> analysis, if we'd really want to go down that road).
We have different depth RSBs for the various uarchs which is what
necessitates the counter. That is, we could always do the max size (32
IIRC) but then, it's expensive and people already complain etc.. etc..
On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
>
>
> On 4/2/20 4:49 PM, Josh Poimboeuf wrote:
> > On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
> > > Hi Alexandre,
> > >
> > > I ran into the limitation of intra-function call for the arm64 support but
> > > didn't take the time to make a clean patch to support them properly.
> >
> > Can you give an example of where arm64 uses intra-function calls? It
> > sounds sketchy to me :-) Is it really needed/useful?
> >
>
> So the most notable/necessary one(s) is the one in tramp_ventry [1]. This
> macro is used as the begining of exception handlers for exceptions coming
> from userland. It was added as part of the mitigations of spectre (v1???).
>
> To give some context, x30 is the register that "ret" instruction will use as
> return address, "bl" is the equivalent of x86 "call" and sets x30 before
> jumping to the target address. (However, it doesn't have a special semantic
> for exception returns)
>
> Note: I believe the comment about the return "stack" is about processor
> internal state (speculative thingies and all) rather than the actual stack,
> since the stack is untouched by that code. But I don't know the actual
> details.
Ok. So another Spectre special case.
> There are also some in arch/arm64/crypto/crct10dif-ce-core.o , which is
> probably full of fast, smart and optimized code I don't understand :) . So I
> wouldn't feel confident commenting on whether those intra-function calls are
> needed or not.
Glancing at that code, there's a macro which has bl to
.L__pmull_p8_core, which, because it has a local label prefix, doesn't
have an ELF symbol associated with it. I bet changing that branch to
"bl __pmull_p8_core" (and removing the unnecessary .L__pmull_p8_core
label) would fix the warnings.
So IIUC, this is actually a case where the warning found a cleanup,
albeit a trivial one.
> Last I found is in qcom_link_stack_sanitization() [2], but that's just a
> workaround for a very specific hardware. In my local tree I just put the
> function as STACK_FRAME_NON_STANDARD. But the code just saves the return
> address, has 16 call instructions that just call the instruction after them,
> restores the return address and lets the C-function return normally (and it
> somehow fixes something for that hardware).
Yeah, like Peter said this sounds like x86 RSB stuffing. More Spectre
nastiness.
So it sounds like the only valid use case for intra-function calls is
Spectre BS...
So, at the risk of possibly contradicting the past version of myself, my
current feeling is that we should annotate such cases, and then warn on
non-annotated cases like the crypto example above. There's nothing
normal about these Spectre mitigations, so it's ok to add annotations to
such craziness.
--
Josh