Jann reported that (for instance) entry_64.o:general_protection has
very odd ORC data:
0000000000000f40 <general_protection>:
#######sp:sp+8 bp:(und) type:iret end:0
f40: 90 nop
#######sp:(und) bp:(und) type:call end:0
f41: 90 nop
f42: 90 nop
#######sp:sp+8 bp:(und) type:iret end:0
f43: e8 a8 01 00 00 callq 10f0 <error_entry>
#######sp:sp+0 bp:(und) type:regs end:0
f48: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp)
f4f: 03
f50: 74 00 je f52 <general_protection+0x12>
f52: 48 89 e7 mov %rsp,%rdi
f55: 48 8b 74 24 78 mov 0x78(%rsp),%rsi
f5a: 48 c7 44 24 78 ff ff movq $0xffffffffffffffff,0x78(%rsp)
f61: ff ff
f63: e8 00 00 00 00 callq f68 <general_protection+0x28>
f68: e9 73 02 00 00 jmpq 11e0 <error_exit>
#######sp:(und) bp:(und) type:call end:0
f6d: 0f 1f 00 nopl (%rax)
Note the entry at 0xf41. Josh found this was the result of commit:
764eef4b109a ("objtool: Rewrite alt->skip_orig")
Due to the early return in validate_branch() we no longer set
insn->cfi of the original instruction stream (the NOPs at 0xf41 and
0xf42) and we'll end up with the above weirdness.
In other discussions we realized alternatives should be ORC invariant;
that is, due to there being only a single ORC table, it must be valid
for all alternatives. The easiest way to ensure this is to not allow
any stack modifications in alternatives.
When we enforce this latter observation, we get the property that the
whole alternative must have the same CFI, which we can employ to fix
the former report.
Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig")
Reported-by: Jann Horn <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
tools/objtool/Documentation/stack-validation.txt | 7 ++++
tools/objtool/check.c | 34 ++++++++++++++++++++++-
2 files changed, 40 insertions(+), 1 deletion(-)
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -315,6 +315,13 @@ they mean, and suggestions for how to fi
function tracing inserts additional calls, which is not obvious from the
sources).
+10. file.o: warning: func()+0x5c: alternative modifies stack
+
+ This means that an alternative includes instructions that modify the
+ stack. The problem is that there is only one ORC unwind table, this means
+ that the ORC unwind entries must be valid for each of the alternatives.
+ The easiest way to enforce this is to ensure alternative do not contain
+ any ORC entries, which in turn implies the above constraint.
If the error doesn't seem to make sense, it could be a bug in objtool.
Feel free to ask the objtool maintainer for help.
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2001,6 +2001,11 @@ static int handle_insn_ops(struct instru
list_for_each_entry(op, &insn->stack_ops, list) {
int res;
+ if (insn->alt_group) {
+ WARN_FUNC("alternative modifies stack", insn->sec, insn->offset);
+ return -1;
+ }
+
res = update_cfi_state(insn, &state->cfi, op);
if (res)
return res;
@@ -2177,6 +2182,30 @@ static bool is_branch_to_alternative(str
}
/*
+ * Alternatives should not contain any ORC entries, this in turn means they
+ * should not contain any CFI ops, which implies all instructions should have
+ * the same same CFI state.
+ *
+ * It is possible to constuct alternatives that have unreachable holes that go
+ * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
+ * states which then results in ORC entries, which we just said we didn't want.
+ *
+ * Avoid them by copying the CFI entry of the first instruction into the whole
+ * alternative.
+ */
+static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
+{
+ struct instruction *first_insn = insn;
+ int alt_group = insn->alt_group;
+
+ sec_for_each_insn_continue(file, insn) {
+ if (insn->alt_group != alt_group)
+ break;
+ insn->cfi = first_insn->cfi;
+ }
+}
+
+/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
* each instruction and validate all the rules described in
@@ -2234,7 +2263,7 @@ static int validate_branch(struct objtoo
insn->visited |= visited;
- if (!insn->ignore_alts) {
+ if (!insn->ignore_alts && !list_empty(&insn->alts)) {
bool skip_orig = false;
list_for_each_entry(alt, &insn->alts, list) {
@@ -2249,6 +2278,9 @@ static int validate_branch(struct objtoo
}
}
+ if (insn->alt_group)
+ fill_alternative_cfi(file, insn);
+
if (skip_orig)
return 0;
}
On Tue, Apr 28, 2020 at 09:11:03PM +0200, Peter Zijlstra wrote:
> Jann reported that (for instance) entry_64.o:general_protection has
> very odd ORC data:
>
> 0000000000000f40 <general_protection>:
> #######sp:sp+8 bp:(und) type:iret end:0
> f40: 90 nop
> #######sp:(und) bp:(und) type:call end:0
> f41: 90 nop
> f42: 90 nop
> #######sp:sp+8 bp:(und) type:iret end:0
> f43: e8 a8 01 00 00 callq 10f0 <error_entry>
> #######sp:sp+0 bp:(und) type:regs end:0
> f48: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp)
> f4f: 03
> f50: 74 00 je f52 <general_protection+0x12>
> f52: 48 89 e7 mov %rsp,%rdi
> f55: 48 8b 74 24 78 mov 0x78(%rsp),%rsi
> f5a: 48 c7 44 24 78 ff ff movq $0xffffffffffffffff,0x78(%rsp)
> f61: ff ff
> f63: e8 00 00 00 00 callq f68 <general_protection+0x28>
> f68: e9 73 02 00 00 jmpq 11e0 <error_exit>
> #######sp:(und) bp:(und) type:call end:0
> f6d: 0f 1f 00 nopl (%rax)
>
> Note the entry at 0xf41. Josh found this was the result of commit:
>
> 764eef4b109a ("objtool: Rewrite alt->skip_orig")
>
> Due to the early return in validate_branch() we no longer set
> insn->cfi of the original instruction stream (the NOPs at 0xf41 and
> 0xf42) and we'll end up with the above weirdness.
>
> In other discussions we realized alternatives should be ORC invariant;
> that is, due to there being only a single ORC table, it must be valid
> for all alternatives. The easiest way to ensure this is to not allow
> any stack modifications in alternatives.
>
> When we enforce this latter observation, we get the property that the
> whole alternative must have the same CFI, which we can employ to fix
> the former report.
>
> Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig")
> Reported-by: Jann Horn <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> tools/objtool/Documentation/stack-validation.txt | 7 ++++
> tools/objtool/check.c | 34 ++++++++++++++++++++++-
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> --- a/tools/objtool/Documentation/stack-validation.txt
> +++ b/tools/objtool/Documentation/stack-validation.txt
> @@ -315,6 +315,13 @@ they mean, and suggestions for how to fi
> function tracing inserts additional calls, which is not obvious from the
> sources).
>
> +10. file.o: warning: func()+0x5c: alternative modifies stack
> +
> + This means that an alternative includes instructions that modify the
> + stack. The problem is that there is only one ORC unwind table, this means
> + that the ORC unwind entries must be valid for each of the alternatives.
> + The easiest way to enforce this is to ensure alternative do not contain
"alternatives"
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Tue, 28 Apr 2020, Peter Zijlstra wrote:
> Jann reported that (for instance) entry_64.o:general_protection has
> very odd ORC data:
>
> 0000000000000f40 <general_protection>:
> #######sp:sp+8 bp:(und) type:iret end:0
> f40: 90 nop
> #######sp:(und) bp:(und) type:call end:0
> f41: 90 nop
> f42: 90 nop
> #######sp:sp+8 bp:(und) type:iret end:0
> f43: e8 a8 01 00 00 callq 10f0 <error_entry>
> #######sp:sp+0 bp:(und) type:regs end:0
> f48: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp)
> f4f: 03
> f50: 74 00 je f52 <general_protection+0x12>
> f52: 48 89 e7 mov %rsp,%rdi
> f55: 48 8b 74 24 78 mov 0x78(%rsp),%rsi
> f5a: 48 c7 44 24 78 ff ff movq $0xffffffffffffffff,0x78(%rsp)
> f61: ff ff
> f63: e8 00 00 00 00 callq f68 <general_protection+0x28>
> f68: e9 73 02 00 00 jmpq 11e0 <error_exit>
> #######sp:(und) bp:(und) type:call end:0
> f6d: 0f 1f 00 nopl (%rax)
>
> Note the entry at 0xf41. Josh found this was the result of commit:
>
> 764eef4b109a ("objtool: Rewrite alt->skip_orig")
>
> Due to the early return in validate_branch() we no longer set
> insn->cfi of the original instruction stream (the NOPs at 0xf41 and
> 0xf42) and we'll end up with the above weirdness.
>
> In other discussions we realized alternatives should be ORC invariant;
> that is, due to there being only a single ORC table, it must be valid
> for all alternatives. The easiest way to ensure this is to not allow
> any stack modifications in alternatives.
>
> When we enforce this latter observation, we get the property that the
> whole alternative must have the same CFI, which we can employ to fix
> the former report.
>
> Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig")
> Reported-by: Jann Horn <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> tools/objtool/Documentation/stack-validation.txt | 7 ++++
> tools/objtool/check.c | 34 ++++++++++++++++++++++-
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> --- a/tools/objtool/Documentation/stack-validation.txt
> +++ b/tools/objtool/Documentation/stack-validation.txt
> @@ -315,6 +315,13 @@ they mean, and suggestions for how to fi
> function tracing inserts additional calls, which is not obvious from the
> sources).
>
> +10. file.o: warning: func()+0x5c: alternative modifies stack
> +
> + This means that an alternative includes instructions that modify the
> + stack. The problem is that there is only one ORC unwind table, this means
> + that the ORC unwind entries must be valid for each of the alternatives.
> + The easiest way to enforce this is to ensure alternative do not contain
> + any ORC entries, which in turn implies the above constraint.
>
> If the error doesn't seem to make sense, it could be a bug in objtool.
> Feel free to ask the objtool maintainer for help.
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2001,6 +2001,11 @@ static int handle_insn_ops(struct instru
> list_for_each_entry(op, &insn->stack_ops, list) {
> int res;
>
> + if (insn->alt_group) {
> + WARN_FUNC("alternative modifies stack", insn->sec, insn->offset);
> + return -1;
> + }
> +
> res = update_cfi_state(insn, &state->cfi, op);
> if (res)
> return res;
> @@ -2177,6 +2182,30 @@ static bool is_branch_to_alternative(str
> }
>
> /*
> + * Alternatives should not contain any ORC entries, this in turn means they
> + * should not contain any CFI ops, which implies all instructions should have
> + * the same same CFI state.
> + *
> + * It is possible to constuct alternatives that have unreachable holes that go
> + * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
> + * states which then results in ORC entries, which we just said we didn't want.
> + *
> + * Avoid them by copying the CFI entry of the first instruction into the whole
> + * alternative.
> + */
> +static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
> +{
> + struct instruction *first_insn = insn;
> + int alt_group = insn->alt_group;
> +
> + sec_for_each_insn_continue(file, insn) {
> + if (insn->alt_group != alt_group)
> + break;
> + insn->cfi = first_insn->cfi;
> + }
> +}
If I am reading this and previous patch correctly...
The function would copy cfi only to "orig" alternative (its insn->alts is
non-empty, orig_insn->alt_group differs from new_insn->alt_group), right?
Would it make sense to do the same for "new" alternative, because of the
invariant? It seems to me it is not processed anywhere that way.
Am I missing something? Whenever I try to read all this alternatives
handling in objtool, I get lost pretty soon.
> +
> +/*
> * Follow the branch starting at the given instruction, and recursively follow
> * any other branches (jumps). Meanwhile, track the frame pointer state at
> * each instruction and validate all the rules described in
> @@ -2234,7 +2263,7 @@ static int validate_branch(struct objtoo
>
> insn->visited |= visited;
>
> - if (!insn->ignore_alts) {
> + if (!insn->ignore_alts && !list_empty(&insn->alts)) {
> bool skip_orig = false;
>
> list_for_each_entry(alt, &insn->alts, list) {
> @@ -2249,6 +2278,9 @@ static int validate_branch(struct objtoo
> }
> }
>
> + if (insn->alt_group)
> + fill_alternative_cfi(file, insn);
> +
fill_alternative_cfi() is called here only for orig_insn, isn't it?
> if (skip_orig)
> return 0;
> }
Miroslav
On Wed, Apr 29, 2020 at 04:33:31PM +0200, Miroslav Benes wrote:
> On Tue, 28 Apr 2020, Peter Zijlstra wrote:
> > /*
> > + * Alternatives should not contain any ORC entries, this in turn means they
> > + * should not contain any CFI ops, which implies all instructions should have
> > + * the same same CFI state.
> > + *
> > + * It is possible to constuct alternatives that have unreachable holes that go
> > + * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
> > + * states which then results in ORC entries, which we just said we didn't want.
> > + *
> > + * Avoid them by copying the CFI entry of the first instruction into the whole
> > + * alternative.
> > + */
> > +static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
> > +{
> > + struct instruction *first_insn = insn;
> > + int alt_group = insn->alt_group;
> > +
> > + sec_for_each_insn_continue(file, insn) {
> > + if (insn->alt_group != alt_group)
> > + break;
> > + insn->cfi = first_insn->cfi;
> > + }
> > +}
>
> If I am reading this and previous patch correctly...
>
> The function would copy cfi only to "orig" alternative (its insn->alts is
> non-empty, orig_insn->alt_group differs from new_insn->alt_group), right?
Yes.
> Would it make sense to do the same for "new" alternative, because of the
> invariant? It seems to me it is not processed anywhere that way.
No.
> Am I missing something? Whenever I try to read all this alternatives
> handling in objtool, I get lost pretty soon.
We only care about the ORC covering the original range, because that is
the range we execute code from. The memory where we store the
alternative instructions (.altinstruction section) is never executed,
that is, RIP should never point there, so we don't need ORC data covering
it.
On Wed, 29 Apr 2020, Peter Zijlstra wrote:
> On Wed, Apr 29, 2020 at 04:33:31PM +0200, Miroslav Benes wrote:
> > On Tue, 28 Apr 2020, Peter Zijlstra wrote:
> > > /*
> > > + * Alternatives should not contain any ORC entries, this in turn means they
> > > + * should not contain any CFI ops, which implies all instructions should have
> > > + * the same same CFI state.
> > > + *
> > > + * It is possible to constuct alternatives that have unreachable holes that go
> > > + * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
> > > + * states which then results in ORC entries, which we just said we didn't want.
> > > + *
> > > + * Avoid them by copying the CFI entry of the first instruction into the whole
> > > + * alternative.
> > > + */
> > > +static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
> > > +{
> > > + struct instruction *first_insn = insn;
> > > + int alt_group = insn->alt_group;
> > > +
> > > + sec_for_each_insn_continue(file, insn) {
> > > + if (insn->alt_group != alt_group)
> > > + break;
> > > + insn->cfi = first_insn->cfi;
> > > + }
> > > +}
> >
> > If I am reading this and previous patch correctly...
> >
> > The function would copy cfi only to "orig" alternative (its insn->alts is
> > non-empty, orig_insn->alt_group differs from new_insn->alt_group), right?
>
> Yes.
>
> > Would it make sense to do the same for "new" alternative, because of the
> > invariant? It seems to me it is not processed anywhere that way.
>
> No.
>
> > Am I missing something? Whenever I try to read all this alternatives
> > handling in objtool, I get lost pretty soon.
>
> We only care about the ORC covering the original range, because that is
> the range we execute code from. The memory where we store the
> alternative instructions (.altinstruction section) is never executed,
> that is, RIP should never point there, so we don't need ORC data covering
> it.
Aha, that's what I didn't realize (again). Note to myself (for the
hundredth time): alternatives are not branches.
Thanks
Miroslav
The following commit has been merged into the objtool/core branch of tip:
Commit-ID: 7117f16bf460ef8cd132e6e80c989677397b4868
Gitweb: https://git.kernel.org/tip/7117f16bf460ef8cd132e6e80c989677397b4868
Author: Peter Zijlstra <[email protected]>
AuthorDate: Tue, 28 Apr 2020 19:37:01 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Thu, 30 Apr 2020 20:14:31 +02:00
objtool: Fix ORC vs alternatives
Jann reported that (for instance) entry_64.o:general_protection has
very odd ORC data:
0000000000000f40 <general_protection>:
#######sp:sp+8 bp:(und) type:iret end:0
f40: 90 nop
#######sp:(und) bp:(und) type:call end:0
f41: 90 nop
f42: 90 nop
#######sp:sp+8 bp:(und) type:iret end:0
f43: e8 a8 01 00 00 callq 10f0 <error_entry>
#######sp:sp+0 bp:(und) type:regs end:0
f48: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp)
f4f: 03
f50: 74 00 je f52 <general_protection+0x12>
f52: 48 89 e7 mov %rsp,%rdi
f55: 48 8b 74 24 78 mov 0x78(%rsp),%rsi
f5a: 48 c7 44 24 78 ff ff movq $0xffffffffffffffff,0x78(%rsp)
f61: ff ff
f63: e8 00 00 00 00 callq f68 <general_protection+0x28>
f68: e9 73 02 00 00 jmpq 11e0 <error_exit>
#######sp:(und) bp:(und) type:call end:0
f6d: 0f 1f 00 nopl (%rax)
Note the entry at 0xf41. Josh found this was the result of commit:
764eef4b109a ("objtool: Rewrite alt->skip_orig")
Due to the early return in validate_branch() we no longer set
insn->cfi of the original instruction stream (the NOPs at 0xf41 and
0xf42) and we'll end up with the above weirdness.
In other discussions we realized alternatives should be ORC invariant;
that is, due to there being only a single ORC table, it must be valid
for all alternatives. The easiest way to ensure this is to not allow
any stack modifications in alternatives.
When we enforce this latter observation, we get the property that the
whole alternative must have the same CFI, which we can employ to fix
the former report.
Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig")
Reported-by: Jann Horn <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
tools/objtool/Documentation/stack-validation.txt | 7 +++-
tools/objtool/check.c | 34 ++++++++++++++-
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/stack-validation.txt
index faa47c3..0189039 100644
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -315,6 +315,13 @@ they mean, and suggestions for how to fix them.
function tracing inserts additional calls, which is not obvious from the
sources).
+10. file.o: warning: func()+0x5c: alternative modifies stack
+
+ This means that an alternative includes instructions that modify the
+ stack. The problem is that there is only one ORC unwind table, this means
+ that the ORC unwind entries must be valid for each of the alternatives.
+ The easiest way to enforce this is to ensure alternatives do not contain
+ any ORC entries, which in turn implies the above constraint.
If the error doesn't seem to make sense, it could be a bug in objtool.
Feel free to ask the objtool maintainer for help.
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4da6bfb..fa9bf36 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1983,6 +1983,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
list_for_each_entry(op, &insn->stack_ops, list) {
int res;
+ if (insn->alt_group) {
+ WARN_FUNC("alternative modifies stack", insn->sec, insn->offset);
+ return -1;
+ }
+
res = update_cfi_state(insn, &state->cfi, op);
if (res)
return res;
@@ -2150,6 +2155,30 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
}
/*
+ * Alternatives should not contain any ORC entries, this in turn means they
+ * should not contain any CFI ops, which implies all instructions should have
+ * the same same CFI state.
+ *
+ * It is possible to constuct alternatives that have unreachable holes that go
+ * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
+ * states which then results in ORC entries, which we just said we didn't want.
+ *
+ * Avoid them by copying the CFI entry of the first instruction into the whole
+ * alternative.
+ */
+static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
+{
+ struct instruction *first_insn = insn;
+ int alt_group = insn->alt_group;
+
+ sec_for_each_insn_continue(file, insn) {
+ if (insn->alt_group != alt_group)
+ break;
+ insn->cfi = first_insn->cfi;
+ }
+}
+
+/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
* each instruction and validate all the rules described in
@@ -2200,7 +2229,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
insn->visited |= visited;
- if (!insn->ignore_alts) {
+ if (!insn->ignore_alts && !list_empty(&insn->alts)) {
bool skip_orig = false;
list_for_each_entry(alt, &insn->alts, list) {
@@ -2215,6 +2244,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}
}
+ if (insn->alt_group)
+ fill_alternative_cfi(file, insn);
+
if (skip_orig)
return 0;
}