[PATCH] ubsan: disable UBSAN_DIV_ZERO for clang

[PATCH] ubsan: disable UBSAN_DIV_ZERO for clang

[Nick Desaulniers]

Building with UBSAN_DIV_ZERO with clang produces numerous fallthrough
warnings from objtool.

In the case of uncheck division, UBSAN_DIV_ZERO may introduce new
control flow to check for division by zero. Because the result of the
division is undefined, LLVM may optimize the control flow such that
after the call to __ubsan_handle_divrem_overflow doesn't matter. If
panic_on_warn was set, __ubsan_handle_divrem_overflow would panic. The
problem is is that panic_on_warn is run time configurable. If it's
disabled, then we cannot guarantee that we will be able to recover
safely.  Disable this config for clang until we can come up with a
solution in LLVM.

Link: https://github.com/ClangBuiltLinux/linux/issues/1657
Link: https://github.com/llvm/llvm-project/issues/56289
Link: https://lore.kernel.org/lkml/CAHk-=wj1qhf7y3VNACEexyp5EbkNpdcu_542k-xZpzmYLOjiCg@mail.gmail.com/
Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Linus,
I still think we should add explicit checks to gaurd against divide by
zero.

 lib/Kconfig.ubsan | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index a9f7eb047768..fd15230a703b 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -84,6 +84,9 @@ config UBSAN_SHIFT
 config UBSAN_DIV_ZERO
 	bool "Perform checking for integer divide-by-zero"
 	depends on $(cc-option,-fsanitize=integer-divide-by-zero)
+	# https://github.com/ClangBuiltLinux/linux/issues/1657
+	# https://github.com/llvm/llvm-project/issues/56289
+	depends on !CC_IS_CLANG
 	help
 	  This option enables -fsanitize=integer-divide-by-zero which checks
 	  for integer division by zero. This is effectively redundant with the
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH] ubsan: disable UBSAN_DIV_ZERO for clang

[Linus Torvalds]

On Thu, Jul 14, 2022 at 1:56 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Linus,
> I still think we should add explicit checks to gaurd against divide by
> zero.

I mean, that's what UBSAN_DIV_ZERO is supposed to do.

The fact that clang then messes it up, and turns "I found undefined
behavior" into "I just crashed the machine" is why it needs to be
disabled.

Please conmvince clang people to fix the sanitizer.

  san·i·tize
  /ˈsanəˌtīz/
  verb
  make clean and hygienic; disinfect.

note how "sanitize" is meant to clean things of undefined behavior.

The way you do that is by warning, and giving it defined behavior. It
really is that simple.

Clang seems to warn and then just turn it into ANOTHER - and much
worse - undefined behavior.

In other words, clang doesn't "sanitize" anything at all. It just
moves the mess around and makes it worse.

                    Linus

Re: [PATCH] ubsan: disable UBSAN_DIV_ZERO for clang

[Nathan Chancellor]

On Thu, Jul 14, 2022 at 01:56:43PM -0700, Nick Desaulniers wrote:
> Building with UBSAN_DIV_ZERO with clang produces numerous fallthrough
> warnings from objtool.
> 
> In the case of uncheck division, UBSAN_DIV_ZERO may introduce new
> control flow to check for division by zero. Because the result of the
> division is undefined, LLVM may optimize the control flow such that
> after the call to __ubsan_handle_divrem_overflow doesn't matter. If
> panic_on_warn was set, __ubsan_handle_divrem_overflow would panic. The
> problem is is that panic_on_warn is run time configurable. If it's
> disabled, then we cannot guarantee that we will be able to recover
> safely.  Disable this config for clang until we can come up with a
> solution in LLVM.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/1657
> Link: https://github.com/llvm/llvm-project/issues/56289
> Link: https://lore.kernel.org/lkml/CAHk-=wj1qhf7y3VNACEexyp5EbkNpdcu_542k-xZpzmYLOjiCg@mail.gmail.com/
> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Acked-by: Nathan Chancellor <nathan@kernel.org>

> ---
> Linus,
> I still think we should add explicit checks to gaurd against divide by
> zero.
> 
>  lib/Kconfig.ubsan | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index a9f7eb047768..fd15230a703b 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -84,6 +84,9 @@ config UBSAN_SHIFT
>  config UBSAN_DIV_ZERO
>  	bool "Perform checking for integer divide-by-zero"
>  	depends on $(cc-option,-fsanitize=integer-divide-by-zero)
> +	# https://github.com/ClangBuiltLinux/linux/issues/1657
> +	# https://github.com/llvm/llvm-project/issues/56289
> +	depends on !CC_IS_CLANG
>  	help
>  	  This option enables -fsanitize=integer-divide-by-zero which checks
>  	  for integer division by zero. This is effectively redundant with the
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 
> 

Re: [PATCH] ubsan: disable UBSAN_DIV_ZERO for clang

[Nick Desaulniers]

On Thu, Jul 14, 2022 at 2:25 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The way you do that is by warning, and giving it defined behavior. It
> really is that simple.

int do_div (int dividend, int divisor) {
  return dividend / divisor;
}

has UB should divisor ever be zero, not much different from:

int deref (int *foo) {
  return *foo;
}

when foo is NULL.  Should the two of those be:

int do_div (int dividend, int divisor) {
  if (!divisor)
    return -EOOPS;
  return dividend / divisor;
}
int deref (int *foo) {
  if (!foo)
    return -EOOPS;
  return *foo;
}

or keep the unchecked versions and wait for a report from a user or
bot with a sanitizer splat?

I get the sanitizer doesn't work as advertised. I _agree_ with you.
Hence this patch (which I _think_ works towards your point, shouldn't
you Ack it?).  I feel like you're talking past me without addressing
my point, let me try rephrasing it:

I _additionally_ think we should be adding more checks to guard
against division by zero to the kernel sources.  Or are we happy to
wait and find out if divisors are ever zero and fix them as they pop
up/become problematic?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] ubsan: disable UBSAN_DIV_ZERO for clang

[Linus Torvalds]

On Thu, Jul 14, 2022 at 2:38 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> int do_div (int dividend, int divisor) {
>   return dividend / divisor;
> }
>
> has UB should divisor ever be zero, not much different from:

So?

What about the million other '/' in the kernel?

Adding one check to 'do_div()' is just stupid. It's like using a
bottle-cap as an umbrella.

Nick, that's the whole _point_ of having compiler support for things
like this - automation. Because doing them manually one at a time is
just completely broken and stupid.

                Linus

Re: [PATCH] ubsan: disable UBSAN_DIV_ZERO for clang

[Linus Torvalds]

On Thu, Jul 14, 2022 at 1:56 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Building with UBSAN_DIV_ZERO with clang produces numerous fallthrough
> warnings from objtool.

Ok, with this applied, things are better.

There are still the "__ubsan_handle_load_invalid_value() with UACCESS
enabled" messages, but those are misfeatures of the kvm cmpxchg
implementation.

I'm not entirely sure why the clang build warns but gcc doesn't, but I
*think* it's because clang is just being silly. It *looks* like it
checks that a "bool" has a value range of 0/1, and will complain if
not.

And the reason I say that's silly is that if I read it correctly, then
that value has literally been generated by clang itself, using "setz"
instruction.

It's the __try_cmpxchg_user_asm() macro, and with clang-14 I have it's
that CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT case, and the C code uses
inline asm and does

        asm_volatile_goto("\n"                                          \
                     "1: " LOCK_PREFIX "cmpxchg"itype" %[new], %[ptr]\n"\
                     _ASM_EXTABLE_UA(1b, %l[label])                     \
                     : CC_OUT(z) (success),                             \

where that CC_OUT() in this case turns into

   # define CC_OUT(c) "=@cc" #c

and clang generates this code for it:

   7d01e:       f0 48 0f b1 4d 00       lock cmpxchg %rcx,0x0(%rbp)
   7d024:       49 89 c5                mov    %rax,%r13
   7d027:       0f 94 c0                sete   %al
   7d02a:       41 88 c6                mov    %al,%r14b
   7d02d:       bf 02 00 00 00          mov    $0x2,%edi
   7d032:       44 89 f6                mov    %r14d,%esi
   7d035:       e8 00 00 00 00          call    __sanitizer_cov_trace_const_cmp1
   7d03a:       41 80 fe 01             cmp    $0x1,%r14b
   7d03e:       0f 87 af 01 00 00       ja     7d1f3
<emulator_cmpxchg_emulated+0x6b3>

where that last "ja     7d1f3" is the branch to the code that then
calls __ubsan_handle_load_invalid_value.

But look at that code: it's literally

    sete   %al
    mov    %al,%r14b
    cmp    $0x1,%r14b

where clang has generated that "sete itself, and then it verifies that
the result is "<= 1".

IOW, clang seems to be literally just checking that the "sete"
instruction works right.

That's silly.

Maybe I'm misreading this, but I think the reason the clang build
complains, but the gcc build does not, is simply because gcc isn't
doing crazy checks of how the CPU works.

Some mis-feature of the "asm with flag output" code, where clang
doesn't understand that it generated that code itself, and the "setcc"
instruction always returns 0/1?

The old issue with "memcpy/memset() leaves .noinstr.text section"
because clang has generated out-of-line functions for trivial copies
also remains, but whatever.

             Linus