linux-hardening - Re: [GIT PULL] Block fixes for 6.3-rc3

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: <CAHk-=wi5yk0+NeqB34fRC-Zvt+8QZVPTiny9MvCxxjg+ZqDhKg@mail.gmail.com>
Date:   Fri, 17 Mar 2023 13:30:46 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Jens Axboe <axboe@...nel.dk>,
        Nathan Chancellor <nathan@...nel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        clang-built-linux <llvm@...ts.linux.dev>,
        linux-hardening@...r.kernel.org
Subject: Re: [GIT PULL] Block fixes for 6.3-rc3

On Fri, Mar 17, 2023 at 12:50 PM Kees Cook <keescook@...omium.org> wrote:
>
> I've hit these cases a few times too. The __ubsan_handle_* stuff
> is designed to be recoverable. I think there are some cases where
> we're tripping over Clang bugs, though. Some of the past issues
> have been with Clang thinking some UBSAN feature was trap-only
> (e.g. -fsanitizer=local-bounds), but here it actually generated the call,
> but decided it was no-return. *sigh*

Hmm. The problem here is that we only get an objdump warning by pure luck.

This isn't a "objdump will always catch it", it's more like "objdump
will only catch it if the code happens to be moved to the end of the
function".

So I'm very happy to hear that it's not by design, and that's very good.

But the whole "this is a compiler bug" doesn't exactly give me the
warm and fuzzies either.

The code generation looks *really* odd to me. This is, as far as I can
tell, the code sequence that this code has:

m5mols_set_fmt:
  ..
        movq    %rdx, %rbp
  ..
        movl    16(%rbp), %ebx
        movq    %rbx, %rdi
  ..
        cmpq    $16385, %rbx                    # imm = 0x4001
        jne     .LBB24_1
  ..
.LBB24_1:
        cmpl    $8199, %ebx                     # imm = 0x2007
        jne     .LBB24_3
  ..
.LBB24_3:
        callq   __sanitizer_cov_trace_pc@PLT
        movl    $2, %esi
        movq    $.L__unnamed_3, %rdi
        callq   __ubsan_handle_out_of_bounds

and as far as I can tell, that "movl 16(%rbp), %ebx" is

        enum m5mols_restype stype = __find_restype(mf->code);

in __find_resolution(). But I don't understand those odd constants
it's comparing against at all.

'enum m5mols_restype' should be in the range 0..2, but it seems to use
a 64-bit compare against '16385' (and then a 32-bit compare against
'8199') to decide it's out-of-bounds.

I must be *completely* mis-reading this, because none of that makes a
whit of sense to me.

It's possible that clang is just *so* terminally confused that it just
generates random code, but it's more likely that I'm mis-reading
things.

The above is my interpretation of what I see with the current F37
clang version for a "allmodconfig" build of that file.

My 'clang -v' says

    clang version 15.0.7 (Fedora 15.0.7-1.fc37)

in case some clang person wants to try to recreate this.

> I'm not opposed to disabling UBSAN for all*config builds if we need to,
> but I want to get these Clang bugs found and fixed so I'd be sad to lose
> the coverage.

I wonder how many people actually end up having UBSAN actually
enabled. I get the feeling that most of the coverage it gets is
exactly just the compile-only coverage by "allmodconfig" and friends.

Otherwise it would be really easy to just say

        depends on !COMPILE_TEST

for all these run-time debug things. But exactly *because* they have
caused endless amounts of build-time pain - *and* because I doubt how
much real run-time testing they get - limiting it to non-build tests
sounds a bit counter-productive.

              Linus