To allow a valid stack unwinding, an alternative should have code
where the same stack changes happens at the same places as in the
original code. Add a check in objtool to validate that stack changes
in alternative are effectively consitent with the original code.
Signed-off-by: Alexandre Chartre <[email protected]>
---
tools/objtool/check.c | 155 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 155 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0574ce8e232d..9488a9c7be24 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2724,6 +2724,156 @@ static int validate_reachable_instructions(struct objtool_file *file)
return 0;
}
+static int validate_alternative_state(struct objtool_file *file,
+ struct instruction *first,
+ struct instruction *prev,
+ struct instruction *current,
+ bool include_current)
+{
+ struct instruction *alt_insn, *alt_first;
+ unsigned long roff_prev, roff_curr, roff;
+ int count, err = 0, err_alt, alt_num;
+ struct alternative *alt;
+ const char *err_str;
+
+ /*
+ * Check that instructions in alternatives between the corresponding
+ * previous and current instructions, have the same stack state.
+ */
+
+ /* use relative offset to find corresponding alt instructions */
+ roff_prev = prev->offset - first->offset;
+ roff_curr = current->offset - first->offset;
+ alt_num = 0;
+
+ list_for_each_entry(alt, &first->alts, list) {
+ if (!alt->insn->alt_group)
+ continue;
+
+ alt_num++;
+ alt_first = alt->insn;
+ count = 0;
+ err_alt = 0;
+
+ for (alt_insn = alt_first;
+ alt_insn && alt_insn->alt_group == alt_first->alt_group;
+ alt_insn = next_insn_same_sec(file, alt_insn)) {
+
+ roff = alt_insn->offset - alt_first->offset;
+ if (roff < roff_prev)
+ continue;
+
+ if (roff > roff_curr ||
+ (roff == roff_curr && !include_current))
+ break;
+
+ count++;
+ /*
+ * The first instruction we check must be aligned with
+ * the corresponding "prev" instruction because stack
+ * change should happen at the same offset.
+ */
+ if (count == 1 && roff != roff_prev) {
+ err_str = "misaligned alternative state change";
+ err_alt++;
+ }
+
+ if (!err_alt && memcmp(&prev->state, &alt_insn->state,
+ sizeof(struct insn_state))) {
+ err_str = "invalid alternative state";
+ err_alt++;
+ }
+
+ /*
+ * On error, report the error and stop checking
+ * this alternative but continue checking other
+ * alternatives.
+ */
+ if (err_alt) {
+ if (!err) {
+ WARN_FUNC("error in alternative",
+ first->sec, first->offset);
+ }
+ WARN_FUNC("in alternative %d",
+ alt_first->sec, alt_first->offset,
+ alt_num);
+ WARN_FUNC("%s", alt_insn->sec, alt_insn->offset,
+ err_str);
+
+ err += err_alt;
+ break;
+ }
+ }
+ }
+
+ return err;
+}
+
+static int validate_alternative(struct objtool_file *file)
+{
+ struct instruction *first, *prev, *next, *insn;
+ bool in_alternative;
+ int err;
+
+ err = 0;
+ first = prev = NULL;
+ in_alternative = false;
+ for_each_insn(file, insn) {
+ if (insn->ignore || !insn->alt_group || insn->ignore_alts)
+ continue;
+
+ if (!in_alternative) {
+ if (list_empty(&insn->alts))
+ continue;
+
+ /*
+ * Setup variables to start the processing of a
+ * new alternative.
+ */
+ first = insn;
+ prev = insn;
+ err = 0;
+ in_alternative = true;
+
+ } else if (!err && memcmp(&prev->state, &insn->state,
+ sizeof(struct insn_state))) {
+ /*
+ * The stack state has changed and no error was
+ * reported for this alternative. Check that the
+ * stack state is the same in all alternatives
+ * between the last check and up to this instruction.
+ *
+ * Once we have an error, stop checking the
+ * alternative because once the stack state is
+ * inconsistent, it will likely be inconsistent
+ * for other instructions as well.
+ */
+ err = validate_alternative_state(file, first,
+ prev, insn, false);
+ prev = insn;
+ }
+
+ /*
+ * If the next instruction is in the same alternative then
+ * continue with processing this alternative. Otherwise
+ * that's the end of this alternative so check there is no
+ * stack state changes in remaining instructions (if no
+ * error was reported yet).
+ */
+ next = list_next_entry(insn, list);
+ if (next && !next->ignore &&
+ next->alt_group == first->alt_group)
+ continue;
+
+ if (!err)
+ err = validate_alternative_state(file, first,
+ prev, insn, true);
+ in_alternative = false;
+ }
+
+ return 0;
+}
+
static struct objtool_file file;
int check(const char *_objname, bool orc)
@@ -2769,6 +2919,11 @@ int check(const char *_objname, bool orc)
goto out;
warnings += ret;
+ ret = validate_alternative(&file);
+ if (ret < 0)
+ goto out;
+ warnings += ret;
+
if (!warnings) {
ret = validate_reachable_instructions(&file);
if (ret < 0)
--
2.18.2
Hi Alexandre,
On 4/14/20 11:36 AM, Alexandre Chartre wrote:
> To allow a valid stack unwinding, an alternative should have code
> where the same stack changes happens at the same places as in the
> original code. Add a check in objtool to validate that stack changes
> in alternative are effectively consitent with the original code.
>
> Signed-off-by: Alexandre Chartre <[email protected]>
> ---
> tools/objtool/check.c | 155 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 155 insertions(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0574ce8e232d..9488a9c7be24 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2724,6 +2724,156 @@ static int validate_reachable_instructions(struct objtool_file *file)
> return 0;
> }
>
> +static int validate_alternative_state(struct objtool_file *file,
> + struct instruction *first,
> + struct instruction *prev,
> + struct instruction *current,
> + bool include_current)
> +{
> + struct instruction *alt_insn, *alt_first;
> + unsigned long roff_prev, roff_curr, roff;
> + int count, err = 0, err_alt, alt_num;
> + struct alternative *alt;
> + const char *err_str;
> +
> + /*
> + * Check that instructions in alternatives between the corresponding
> + * previous and current instructions, have the same stack state.
> + */
> +
> + /* use relative offset to find corresponding alt instructions */
> + roff_prev = prev->offset - first->offset;
> + roff_curr = current->offset - first->offset;
> + alt_num = 0;
> +
> + list_for_each_entry(alt, &first->alts, list) {
> + if (!alt->insn->alt_group)
> + continue;
> +
> + alt_num++;
> + alt_first = alt->insn;
> + count = 0;
> + err_alt = 0;
Unless I'm missing something, err_alt is wither 1 or 0, so perhaps a
boolean would be more appropriate.
> +
> + for (alt_insn = alt_first;
> + alt_insn && alt_insn->alt_group == alt_first->alt_group;
> + alt_insn = next_insn_same_sec(file, alt_insn)) {
> +
> + roff = alt_insn->offset - alt_first->offset;
> + if (roff < roff_prev)
> + continue;
> +
> + if (roff > roff_curr ||
> + (roff == roff_curr && !include_current))
> + break;
> +
> + count++;
> + /*
> + * The first instruction we check must be aligned with
> + * the corresponding "prev" instruction because stack
> + * change should happen at the same offset.
> + */
> + if (count == 1 && roff != roff_prev) {
> + err_str = "misaligned alternative state change";
> + err_alt++;
> + }
> +
> + if (!err_alt && memcmp(&prev->state, &alt_insn->state,
> + sizeof(struct insn_state))) {
In insn_state_match(), not the whole of the insn_state is taken into
account. In particular, uaccess_stack, uaccess, df and end are not
compared. Also, stack_size (but maybe that should be included in
insn_state_match() ) and vals are not checked.
Is there a reason we'd check those for alternatives but not in the
normal case? And does your alternative validation work with uaccess check?
> + err_str = "invalid alternative state";
> + err_alt++;
> + }
> +
> + /*
> + * On error, report the error and stop checking
> + * this alternative but continue checking other
> + * alternatives.
> + */
> + if (err_alt) {
> + if (!err) {
> + WARN_FUNC("error in alternative",
> + first->sec, first->offset);
> + }
> + WARN_FUNC("in alternative %d",
> + alt_first->sec, alt_first->offset,
> + alt_num);
> + WARN_FUNC("%s", alt_insn->sec, alt_insn->offset,
> + err_str);
> +
> + err += err_alt;
> + break;
> + }
> + }
> + }
> +
> + return err;
> +}
> +
> +static int validate_alternative(struct objtool_file *file)
> +{
> + struct instruction *first, *prev, *next, *insn;
> + bool in_alternative;
> + int err;
> +
> + err = 0;
> + first = prev = NULL;
> + in_alternative = false;
> + for_each_insn(file, insn) {
> + if (insn->ignore || !insn->alt_group || insn->ignore_alts)
> + continue;
> +
> + if (!in_alternative) {
> + if (list_empty(&insn->alts))
> + continue;
> +
> + /*
> + * Setup variables to start the processing of a
> + * new alternative.
> + */
> + first = insn;
> + prev = insn;
> + err = 0;
> + in_alternative = true;
> +
> + } else if (!err && memcmp(&prev->state, &insn->state,
> + sizeof(struct insn_state))) {
> + /*
> + * The stack state has changed and no error was
> + * reported for this alternative. Check that the
> + * stack state is the same in all alternatives
> + * between the last check and up to this instruction.
> + *
> + * Once we have an error, stop checking the
> + * alternative because once the stack state is
> + * inconsistent, it will likely be inconsistent
> + * for other instructions as well.
> + */
> + err = validate_alternative_state(file, first,
> + prev, insn, false);
> + prev = insn;
> + }
> +
> + /*
> + * If the next instruction is in the same alternative then
> + * continue with processing this alternative. Otherwise
> + * that's the end of this alternative so check there is no
> + * stack state changes in remaining instructions (if no
> + * error was reported yet).
> + */
> + next = list_next_entry(insn, list);
> + if (next && !next->ignore &&
> + next->alt_group == first->alt_group)
> + continue;
> +
> + if (!err)
> + err = validate_alternative_state(file, first,
> + prev, insn, true);
> + in_alternative = false;
> + }
> +
> + return 0;
> +}
> +
> static struct objtool_file file;
>
> int check(const char *_objname, bool orc)
> @@ -2769,6 +2919,11 @@ int check(const char *_objname, bool orc)
> goto out;
> warnings += ret;
>
> + ret = validate_alternative(&file);
> + if (ret < 0)
> + goto out;
> + warnings += ret;
> +
> if (!warnings) {
> ret = validate_reachable_instructions(&file);
> if (ret < 0)
>
Cheers,
--
Julien Thierry
Hi Alexandre,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/auto-latest]
[also build test WARNING on linus/master v5.7-rc1 next-20200414]
[cannot apply to tip/x86/core linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Alexandre-Chartre/objtool-changes-to-check-retpoline-code/20200415-015312
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 354a2fb98a37f696a1cffdc58dbff48a02e7c611
config: x86_64-randconfig-b002-20200415 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> mm/kasan/common.o: warning: objtool: kasan_report()+0x13: error in alternative
>> mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xa: in alternative 1
>> mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xb: invalid alternative state
mm/kasan/common.o: warning: objtool: kasan_report()+0x3b: error in alternative
mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xf: in alternative 1
mm/kasan/common.o: warning: objtool: .altinstr_replacement+0x10: invalid alternative state
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Alexandre,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/auto-latest]
[also build test WARNING on linus/master v5.7-rc1 next-20200414]
[cannot apply to tip/x86/core linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Alexandre-Chartre/objtool-changes-to-check-retpoline-code/20200415-015312
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 354a2fb98a37f696a1cffdc58dbff48a02e7c611
config: x86_64-randconfig-g003-20200414 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> kernel/trace/trace_branch.o: warning: objtool: ftrace_likely_update()+0x4: error in alternative
>> kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x0: in alternative 1
>> kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x1: invalid alternative state
kernel/trace/trace_branch.o: warning: objtool: ftrace_likely_update()+0x21: error in alternative
kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x5: in alternative 1
kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x6: invalid alternative state
--
>> lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0xc: error in alternative
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x0: in alternative 1
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x1: invalid alternative state
lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0x10a: error in alternative
lib/ubsan.o: warning: objtool: .altinstr_replacement+0x6: in alternative 1
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change
>> lib/ubsan.o: warning: objtool: __ubsan_handle_shift_out_of_bounds()+0x2b: error in alternative
lib/ubsan.o: warning: objtool: .altinstr_replacement+0x9: in alternative 1
lib/ubsan.o: warning: objtool: .altinstr_replacement+0xa: invalid alternative state
lib/ubsan.o: warning: objtool: __ubsan_handle_shift_out_of_bounds()+0x103: error in alternative
lib/ubsan.o: warning: objtool: .altinstr_replacement+0xf: in alternative 1
lib/ubsan.o: warning: objtool: .altinstr_replacement+0x11: misaligned alternative state change
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 4/16/20 4:18 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 12:36:15PM +0200, Alexandre Chartre wrote:
>> To allow a valid stack unwinding, an alternative should have code
>> where the same stack changes happens at the same places as in the
>> original code. Add a check in objtool to validate that stack changes
>> in alternative are effectively consitent with the original code.
>
> This thing is completely buggered, it warns all over the place, even for
> obviously correct alternatives like:
>
> 0000000000000310 <return_to_handler>:
> 310: 48 83 ec 18 sub $0x18,%rsp
> 314: 48 89 04 24 mov %rax,(%rsp)
> 318: 48 89 54 24 08 mov %rdx,0x8(%rsp)
> 31d: 48 89 ef mov %rbp,%rdi
> 320: e8 00 00 00 00 callq 325 <return_to_handler+0x15>
> 321: R_X86_64_PLT32 ftrace_return_to_handler-0x4
> 325: 48 89 c7 mov %rax,%rdi
> 328: 48 8b 54 24 08 mov 0x8(%rsp),%rdx
> 32d: 48 8b 04 24 mov (%rsp),%rax
> 331: 48 83 c4 18 add $0x18,%rsp
> 335: ff e7 jmpq *%rdi
> 337: 90 nop
> 338: 90 nop
> 339: 90 nop
>
>
> Where 335 has two alternatives:
>
> 0: e9 00 00 00 00 jmpq 5 <.altinstr_replacement+0x5>
> 1: R_X86_64_PLT32 __x86_retpoline_rdi-0x4
>
> and
>
> 5: 0f ae e8 lfence
> 8: ff e7 jmpq *%rdi
>
>
> And it then comes back with:
>
> defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .entry.text+0x335: error in alternative
> defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x5: in alternative 2
> defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change
>
> which is just utter crap, JMP has no (CFI) state change.
I think that's because the original nop instructions are not reached, so
they probably have an undefined stack state. Hence there's a different
stack state between 335 (jmpq *%rdi) and 337 (nop).
With the alternative, we have:
335: lfence
338: jmpq *%rdi
So now instruction 338 is reached with the stack state we had at 335
which is different from the original stack state at 338 (undefined)
with the unreached instruction.
Don't we have a state change recorded at 337 because the instruction
is seen as not reached?
alex.
On Tue, Apr 14, 2020 at 12:36:15PM +0200, Alexandre Chartre wrote:
> To allow a valid stack unwinding, an alternative should have code
> where the same stack changes happens at the same places as in the
> original code. Add a check in objtool to validate that stack changes
> in alternative are effectively consitent with the original code.
This thing is completely buggered, it warns all over the place, even for
obviously correct alternatives like:
0000000000000310 <return_to_handler>:
310: 48 83 ec 18 sub $0x18,%rsp
314: 48 89 04 24 mov %rax,(%rsp)
318: 48 89 54 24 08 mov %rdx,0x8(%rsp)
31d: 48 89 ef mov %rbp,%rdi
320: e8 00 00 00 00 callq 325 <return_to_handler+0x15>
321: R_X86_64_PLT32 ftrace_return_to_handler-0x4
325: 48 89 c7 mov %rax,%rdi
328: 48 8b 54 24 08 mov 0x8(%rsp),%rdx
32d: 48 8b 04 24 mov (%rsp),%rax
331: 48 83 c4 18 add $0x18,%rsp
335: ff e7 jmpq *%rdi
337: 90 nop
338: 90 nop
339: 90 nop
Where 335 has two alternatives:
0: e9 00 00 00 00 jmpq 5 <.altinstr_replacement+0x5>
1: R_X86_64_PLT32 __x86_retpoline_rdi-0x4
and
5: 0f ae e8 lfence
8: ff e7 jmpq *%rdi
And it then comes back with:
defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .entry.text+0x335: error in alternative
defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x5: in alternative 2
defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change
which is just utter crap, JMP has no (CFI) state change.