On Tue, Sep 15, 2020 at 01:12:24PM -0700, Nick Desaulniers wrote:
> 1 warning: objtool: ist_exc_vmm_communication()+0x12: unreachable instruction
That looks interesting. So your .o has:
00000000000004c0 <ist_exc_vmm_communication>:
4c0: 55 push %rbp
4c1: 48 89 e5 mov %rsp,%rbp
4c4: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
4cb: 31 c0 xor %eax,%eax
4cd: e8 00 00 00 00 callq 4d2 <ist_exc_vmm_communication+0x12>
4d2: 0f 0b ud2
4d4: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
4db: 00 00 00 00
4df: 90 nop
And the unreachable insn is at 0x4d2. The version I got when building with
clang12 built from git of today is:
00000000000003e0 <ist_exc_vmm_communication>:
3e0: 55 push %rbp
3e1: 48 89 e5 mov %rsp,%rbp
3e4: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
3eb: 31 c0 xor %eax,%eax
3ed: e8 00 00 00 00 callq 3f2 <ist_exc_vmm_communication+0x12>
3f2: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
3f9: 00 00 00 00
3fd: 0f 1f 00 nopl (%rax)
and that version is calling a bunch of NOPs.
gcc produces:
00000000000002aa <ist_exc_vmm_communication>:
2aa: 55 push %rbp
2ab: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
2b2: 48 89 e5 mov %rsp,%rbp
2b5: e8 00 00 00 00 callq 2ba <ist_exc_vmm_communication+0x10>
2ba: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
(Btw, clang doesn't need to add that "xor %eax,%eax" - panic() should not be
returning, ever. :-))
So what that call actually is, is:
# arch/x86/kernel/sev-es.c:1342: panic("Can't handle #VC exception from unsupported context\n");
call panic #
and the address of panic() gets fixed up by the linker into:
ffffffff83066dca <ist_exc_vmm_communication>:
ffffffff83066dca: 55 push %rbp
ffffffff83066dcb: 48 c7 c7 08 4f e2 83 mov $0xffffffff83e24f08,%rdi
ffffffff83066dd2: 48 89 e5 mov %rsp,%rbp
ffffffff83066dd5: e8 52 23 ff ff callq ffffffff8305912c <panic>
ffffffff83066dda: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
But your compiler generates a call to UD2.
Interesting.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Sep 15, 2020 at 10:49:12PM +0200, Borislav Petkov wrote:
> On Tue, Sep 15, 2020 at 01:12:24PM -0700, Nick Desaulniers wrote:
> > 1 warning: objtool: ist_exc_vmm_communication()+0x12: unreachable instruction
>
> That looks interesting. So your .o has:
>
> 00000000000004c0 <ist_exc_vmm_communication>:
> 4c0: 55 push %rbp
> 4c1: 48 89 e5 mov %rsp,%rbp
> 4c4: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
> 4cb: 31 c0 xor %eax,%eax
> 4cd: e8 00 00 00 00 callq 4d2 <ist_exc_vmm_communication+0x12>
> 4d2: 0f 0b ud2
> 4d4: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 4db: 00 00 00 00
> 4df: 90 nop
If you disassemble with "objdump -dr" it shows the relocations:
00000000000004c0 <ist_exc_vmm_communication>:
4c0: 55 push %rbp
4c1: 48 89 e5 mov %rsp,%rbp
4c4: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
4c7: R_X86_64_32S .rodata.str1.1+0x1ef
4cb: 31 c0 xor %eax,%eax
4cd: e8 00 00 00 00 callq 4d2 <ist_exc_vmm_communication+0x12>
4ce: R_X86_64_PLT32 panic-0x4
4d2: 0f 0b ud2
4d4: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
4db: 00 00 00 00
4df: 90 nop
panic() is noreturn, so the compiler is enforcing the fact that it
doesn't return, by trapping if it does return.
I seem to remember that's caused by CONFIG_UBSAN_TRAP.
--
Josh
On Tue, Sep 15, 2020 at 04:02:31PM -0500, Josh Poimboeuf wrote:
> panic() is noreturn, so the compiler is enforcing the fact that it
> doesn't return, by trapping if it does return.
>
> I seem to remember that's caused by CONFIG_UBSAN_TRAP.
From IRC: yah, CONFIG_UBSAN_TRAP=y in that config. But why doesn't my
clang12 generate this ud2?
gcc doesn't do that either.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Sep 15, 2020 at 10:49:12PM +0200, Borislav Petkov wrote:
>
> (Btw, clang doesn't need to add that "xor %eax,%eax" - panic() should not be
> returning, ever. :-))
>
I think this is because panic() is varargs, and clang doesn't support
gcc's -mskip-rax-setup. The normal ABI requires the caller to set RAX to
the number of arguments in vector registers.
https://patchwork.ozlabs.org/project/gcc/patch/[email protected]/
On Tue, Sep 15, 2020 at 2:50 PM Arvind Sankar <[email protected]> wrote:
>
> On Tue, Sep 15, 2020 at 10:49:12PM +0200, Borislav Petkov wrote:
> >
> > (Btw, clang doesn't need to add that "xor %eax,%eax" - panic() should not be
> > returning, ever. :-))
> >
>
> I think this is because panic() is varargs, and clang doesn't support
> gcc's -mskip-rax-setup. The normal ABI requires the caller to set RAX to
> the number of arguments in vector registers.
>
> https://patchwork.ozlabs.org/project/gcc/patch/[email protected]/
Thanks for the report. Filed
https://bugs.llvm.org/show_bug.cgi?id=47538. Do you have an account
there Arvind so that I can CC you on them?
Yikes, it looks like gcc 5.0 supports that. We'd better get to
implementing that lest someone try to remove the cc-option on it soon.
--
Thanks,
~Nick Desaulniers
On Tue, Sep 15, 2020 at 2:02 PM Josh Poimboeuf <[email protected]> wrote:
>
> panic() is noreturn, so the compiler is enforcing the fact that it
> doesn't return, by trapping if it does return.
>
> I seem to remember that's caused by CONFIG_UBSAN_TRAP.
Indeed, if I remove CONFIG_UBSAN_TRAP from the 0day report's
randconfig, these unreachable instruction warnings all go away.
So what's the right way to fix this?
CONFIG_UBSAN_TRAP enables -fsanitize-undefined-trap-on-error (not
sure why that's wrapped in cc-option; it shouldn't be selectable via
Kconfig if unsupported by the toolchain).
Should clang not be emitting `ud2` trapping instructions for this flag
for no-return functions?
or
Should objtool be made aware of the config option and then not check
traps after no-returns?
I suspect the latter, but I'm not sure how feasible it is to
implement. Josh, Marco, do you have thoughts on the above?
--
Thanks,
~Nick Desaulniers
On Tue, Sep 15, 2020 at 02:59:19PM -0700, Nick Desaulniers wrote:
> On Tue, Sep 15, 2020 at 2:50 PM Arvind Sankar <[email protected]> wrote:
> >
> > On Tue, Sep 15, 2020 at 10:49:12PM +0200, Borislav Petkov wrote:
> > >
> > > (Btw, clang doesn't need to add that "xor %eax,%eax" - panic() should not be
> > > returning, ever. :-))
> > >
> >
> > I think this is because panic() is varargs, and clang doesn't support
> > gcc's -mskip-rax-setup. The normal ABI requires the caller to set RAX to
> > the number of arguments in vector registers.
> >
> > https://patchwork.ozlabs.org/project/gcc/patch/[email protected]/
>
> Thanks for the report. Filed
> https://bugs.llvm.org/show_bug.cgi?id=47538. Do you have an account
> there Arvind so that I can CC you on them?
Nope, no bugzilla account.
If you're adding flags, another set the kernel uses is the
-falign-{jumps,labels,loops} to turn off alignment of jumps and loops.
>
> Yikes, it looks like gcc 5.0 supports that. We'd better get to
> implementing that lest someone try to remove the cc-option on it soon.
> --
> Thanks,
> ~Nick Desaulniers
> Should objtool be made aware of the config option and then not check
> traps after no-returns?
>
> I suspect the latter, but I'm not sure how feasible it is to
> implement. Josh, Marco, do you have thoughts on the above?
This seems to do the trick.
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e034a8f24f46..9224e6565ba2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2612,9 +2612,10 @@ static bool is_ubsan_insn(struct instruction *insn)
"__ubsan_handle_builtin_unreachable"));
}
-static bool ignore_unreachable_insn(struct instruction *insn)
+static bool ignore_unreachable_insn(struct objtool_file *file, struct
instruction *insn)
{
int i;
+ struct instruction *prev_insn;
if (insn->ignore || insn->type == INSN_NOP)
return true;
@@ -2640,7 +2641,8 @@ static bool ignore_unreachable_insn(struct
instruction *insn)
* the UD2, which causes GCC's undefined trap logic to emit another UD2
* (or occasionally a JMP to UD2).
*/
- if (list_prev_entry(insn, list)->dead_end &&
+ prev_insn = list_prev_entry(insn, list);
+ if ((prev_insn->dead_end || dead_end_function(file,
prev_insn->call_dest)) &&
(insn->type == INSN_BUG ||
(insn->type == INSN_JUMP_UNCONDITIONAL &&
insn->jump_dest && insn->jump_dest->type == INSN_BUG)))
@@ -2767,7 +2769,7 @@ static int
validate_reachable_instructions(struct objtool_file *file)
return 0;
for_each_insn(file, insn) {
- if (insn->visited || ignore_unreachable_insn(insn))
+ if (insn->visited || ignore_unreachable_insn(file, insn))
continue;
WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
On Wed, 16 Sep 2020 at 00:34, Nick Desaulniers <[email protected]> wrote:
> On Tue, Sep 15, 2020 at 2:02 PM Josh Poimboeuf <[email protected]> wrote:
> >
> > panic() is noreturn, so the compiler is enforcing the fact that it
> > doesn't return, by trapping if it does return.
> >
> > I seem to remember that's caused by CONFIG_UBSAN_TRAP.
>
> Indeed, if I remove CONFIG_UBSAN_TRAP from the 0day report's
> randconfig, these unreachable instruction warnings all go away.
>
> So what's the right way to fix this?
>
> CONFIG_UBSAN_TRAP enables -fsanitize-undefined-trap-on-error (not
> sure why that's wrapped in cc-option; it shouldn't be selectable via
> Kconfig if unsupported by the toolchain).
>
> Should clang not be emitting `ud2` trapping instructions for this flag
> for no-return functions?
I think this would defeat the purpose of this UBSAN feature. Certain
UBSAN checks are done fully statically, like is done by
fsanitize=unreachable, and could actually be enabled in production
kernels; trapping the kernel in these cases would be a reasonable way
to avoid further damage to the system.
(You could in theory force it to not emit a trap by using
fno-sanitize-trap=unreachable, but I think it's a bad idea.)
> or
>
> Should objtool be made aware of the config option and then not check
> traps after no-returns?
I'd vote for this. And it seems Ilie implemented this already.
> I suspect the latter, but I'm not sure how feasible it is to
> implement. Josh, Marco, do you have thoughts on the above?
Thanks,
-- Marco
On Tue, Sep 15, 2020 at 05:50:54PM -0400, Arvind Sankar wrote:
> On Tue, Sep 15, 2020 at 10:49:12PM +0200, Borislav Petkov wrote:
> >
> > (Btw, clang doesn't need to add that "xor %eax,%eax" - panic() should not be
> > returning, ever. :-))
> >
>
> I think this is because panic() is varargs, and clang doesn't support
> gcc's -mskip-rax-setup. The normal ABI requires the caller to set RAX to
> the number of arguments in vector registers.
>
> https://patchwork.ozlabs.org/project/gcc/patch/[email protected]/
Ah, good point. Found this in the ABI doc:
"For calls that may call functions that use varargs or stdargs
(prototype-less calls or calls to functions containing ellipsis (...) in
the declaration) %al is used as hidden argument to specify the number of
vector registers used. The contents of %al do not need to match exactly
the number of registers, but must be an upper bound on the number of
vector registers used and is in the range 0–8 inclusive."
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
The following commit has been merged into the objtool/core branch of tip:
Commit-ID: 14db1f0a93331d0958e90da522c429ff0890d2d6
Gitweb: https://git.kernel.org/tip/14db1f0a93331d0958e90da522c429ff0890d2d6
Author: Ilie Halip <[email protected]>
AuthorDate: Sat, 19 Sep 2020 09:41:18 +03:00
Committer: Josh Poimboeuf <[email protected]>
CommitterDate: Mon, 21 Sep 2020 10:20:10 -05:00
objtool: Ignore unreachable trap after call to noreturn functions
With CONFIG_UBSAN_TRAP enabled, the compiler may insert a trap
instruction after a call to a noreturn function. In this case, objtool
warns that the UD2 instruction is unreachable.
This is a behavior seen with Clang, from the oldest version capable of
building the mainline x64_64 kernel (9.0), to the latest experimental
version (12.0).
Objtool silences similar warnings (trap after dead end instructions), so
so expand that check to include dead end functions.
Cc: Nick Desaulniers <[email protected]>
Cc: Rong Chen <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Philip Li <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
BugLink: https://github.com/ClangBuiltLinux/linux/issues/1148
Link: https://lore.kernel.org/lkml/CAKwvOdmptEpi8fiOyWUo=AiZJiX+Z+VHJOM2buLPrWsMTwLnyw@mail.gmail.com
Suggested-by: Nick Desaulniers <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
Tested-by: Nick Desaulniers <[email protected]>
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Ilie Halip <[email protected]>
Tested-by: Sedat Dilek <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a4796e3..2df9f76 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2638,9 +2638,10 @@ static bool is_ubsan_insn(struct instruction *insn)
"__ubsan_handle_builtin_unreachable"));
}
-static bool ignore_unreachable_insn(struct instruction *insn)
+static bool ignore_unreachable_insn(struct objtool_file *file, struct instruction *insn)
{
int i;
+ struct instruction *prev_insn;
if (insn->ignore || insn->type == INSN_NOP)
return true;
@@ -2668,8 +2669,11 @@ static bool ignore_unreachable_insn(struct instruction *insn)
* __builtin_unreachable(). The BUG() macro has an unreachable() after
* the UD2, which causes GCC's undefined trap logic to emit another UD2
* (or occasionally a JMP to UD2).
+ *
+ * It may also insert a UD2 after calling a __noreturn function.
*/
- if (list_prev_entry(insn, list)->dead_end &&
+ prev_insn = list_prev_entry(insn, list);
+ if ((prev_insn->dead_end || dead_end_function(file, prev_insn->call_dest)) &&
(insn->type == INSN_BUG ||
(insn->type == INSN_JUMP_UNCONDITIONAL &&
insn->jump_dest && insn->jump_dest->type == INSN_BUG)))
@@ -2796,7 +2800,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
return 0;
for_each_insn(file, insn) {
- if (insn->visited || ignore_unreachable_insn(insn))
+ if (insn->visited || ignore_unreachable_insn(file, insn))
continue;
WARN_FUNC("unreachable instruction", insn->sec, insn->offset);