linux-kernel - Re: mainline build failure due to 281d0c962752 ("fortify: Add Clang support")

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220628224255.w4lmzalkx3qejuyg@treble>
Date:   Tue, 28 Jun 2022 15:42:55 -0700
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        clang-built-linux <llvm@...ts.linux.dev>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: mainline build failure due to 281d0c962752 ("fortify: Add Clang
 support")

On Thu, Jun 23, 2022 at 04:33:35PM -0700, Nick Desaulniers wrote:
> On Wed, Jun 22, 2022 at 3:40 PM Nick Desaulniers
> <ndesaulniers@...gle.com> wrote:
> >
> > On Wed, Jun 22, 2022 at 10:49 AM Linus Torvalds
> > <torvalds@...ux-foundation.org> wrote:
> > >
> > > On Wed, Jun 22, 2022 at 12:26 PM Sudip Mukherjee
> > > <sudipm.mukherjee@...il.com> wrote:
> > > >
> > > > Tried it after applying your patch. There was no build failure, but some warnings:
> > >
> > > So some of those objtool warnings are, I think, because clang does odd
> > > and crazy things for when it decides "this is not reachable" code.
> > >
> > > I don't much like it, and neither does objtool, but it is what it is.
> > > When clang decides "I'm calling a function that cannot return", it
> > > will have a "call" instruction and then it will just fall off the face
> > > of the earth after that.

FWIW, GCC does the same thing for calls to __noreturn functions.  After
the call it just "falls off the face of the earth".  So there's nothing
special about Clang here.

> Looks like these are actually from calls to
> __ubsan_handle_divrem_overflow which is __noreturn when panic_on_warn
> is set by the corresponding config.  I wonder if we should be
> unconditionally adding __ubsan_handle_divrem_overflow to the allow
> list `global_noreturns` in tools/objtool/check.c?  It seems like the
> kconfig defines aren't passed through to the tools/ sources.
> 
> List of fallthrough warnings from allmodconfig for reference:
> https://lore.kernel.org/lkml/YrNQrPNF%2FXfriP99@debian/

As discussed with Nick on IRC, the problem highlighted by the
fallthrough warnings seems to be a mismatch in expectations as to
whether __ubsan_handle_divrem_overflow() is noreturn.

When Clang inserts calls to it, it assumes it's noreturn.  But in fact
its kernel implementation can actually return, if !panic_on_warn.

So it needs to be changed to *never* return, otherwise hilarity will
ensue when it returns and "falls off the face of the earth".

And then objtool needs to be told that it's in fact noreturn.

So, something like this:


diff --git a/lib/ubsan.c b/lib/ubsan.c
index 36bd75e33426..d257baa62721 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -177,6 +177,7 @@ void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs)
 		pr_err("division by zero\n");
 
 	ubsan_epilogue();
+	panic("can't return from __ubsan_handle_divrem_overflow()");
 }
 EXPORT_SYMBOL(__ubsan_handle_divrem_overflow);
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 864bb9dd3584..6f67d48fb3e4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -187,6 +187,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
 		"__invalid_creds",
 		"cpu_startup_entry",
 		"__ubsan_handle_builtin_unreachable",
+		"__ubsan_handle_divrem_overflow",
 		"ex_handler_msr_mce",
 	};