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]

Date:   Wed, 29 Jun 2022 14:21:25 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Josh Poimboeuf <jpoimboe@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        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 Wed, Jun 29, 2022 at 9:34 AM Josh Poimboeuf <jpoimboe@...nel.org> wrote:
>
> On Wed, Jun 29, 2022 at 09:08:20AM -0700, Linus Torvalds wrote:
> > On Tue, Jun 28, 2022 at 3:43 PM Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > >
> > > So, something like this:
> >
> > No, clang should just be fixed.
> >
> > These UBSAN reports should usually be WARN_ON_ONCE.
> >
> > It's all the same issues we've had before: causing a panic will just
> > kill the machine, and gets us fewer reports.
> >
> > Now, UBSAN is something that presumably normal people don't actually
> > run on real hardware, so it's probably less of a deal than some. But
> > hey, maybe somebody wants to actually run an UBSAN kernel on a real
> > load with a full accelerated graphical UI and real drivers: a panic
> > may end up killing the kernel, and there you sit, with a dead machine
> > and no idea what went wrong.
> >
> > So the whole "panic if UBSAN reports something" is COMPLETE GARBAGE.
> > It actually makes the whole point of running UBSAN go away. You *lose*
> > coverage.
> >
> > So please don't make the kernel worse because clang got something like
> > this wrong.
> >
> > Just fix clang.
> >
> > And fix your mindset.
>
> Yeah, good point.  All the other UBSAN handlers (other than builtin
> unreachable) try to recover.  There's nothing special about divrem
> overflow which requires it to be fatal.
>
> So clang needs to stop assuming the divrem overflow handler is noreturn.

I haven't been able to verify in the generated IR or in LLVM sources
that that is the case; it doesn't appear to be any assumption about
__ubsan_handle_divrem_overflow being noreturn.

Instead, it looks like this has more to do with undefined behavior,
IIUC.  It seems that for `-fsanitize=integer-divide-by-zero`
(CONFIG_UBSAN_DIV_ZERO), we introduce new control flow to warn on div
by zero, but the results of the division are undefined, so
unconditionally trying to consume the result is UB.

So:

int foo (int x, int y) {
  return x / y;
}

doesn't check for divide by zero, -fsanitize=integer-divide-by-zero
introduces control flow to check for divide by zero and call
__ubsan_handle_divrem_overflow in that exceptional case, but the
result of the division is still undefined for that case, so control
flow that tries to use that result is simplified into using the result
in the non-zero divisor case, and calls to
__ubsan_handle_divrem_overflow followed by (what is effectively)
__builtin_unreachable().

For hahas I tried:

```
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index d7aa5511c361..cb2eab660fd2 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -907,8 +907,11 @@ static ssize_t ufx_ops_write(struct fb_info
*info, const char __user *buf,
        result = fb_sys_write(info, buf, count, ppos);

        if (result > 0) {
-               int start = max((int)(offset / info->fix.line_length), 0);
-               int lines = min((u32)((result / info->fix.line_length) + 1),
+               int start, lines;
+               if (!info->fix.line_length)
+                       return 0;
+               start = max((int)(offset / info->fix.line_length), 0);
+               lines = min((u32)((result / info->fix.line_length) + 1),
                                (u32)info->var.yres);

                ufx_handle_damage(dev, 0, start, info->var.xres, lines);
```
That way we don't unconditionally consume the results of a possible
divide by zero, and that fixes the warning from that TU.  Does
inserting guards against divide by zero on the kernel side seem like
reasonable fixes for these warnings?
-- 
Thanks,
~Nick Desaulniers