Message-ID: <CAKwvOd=MQmDMSpoLb3c081+wGAvGr3hHE0uGgXYm=FrvEprDMw@mail.gmail.com>
Date:   Fri, 17 Mar 2023 13:35:56 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Kees Cook <keescook@...omium.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     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:
>
> On Fri, Mar 17, 2023 at 11:59:11AM -0700, Nick Desaulniers wrote:
> > +Sanitizer folks (BCC'd)
> > (Top of lore thread:
> > https://lore.kernel.org/linux-block/9d0ef355-f430-e8e2-c844-b34cfcf60d88@kernel.dk/)
> >
> > On Fri, Mar 17, 2023 at 11:35 AM Linus Torvalds
> > <torvalds@...ux-foundation.org> wrote:
> > >
> > > On Fri, Mar 17, 2023 at 10:16 AM Jens Axboe <axboe@...nel.dk> wrote:
> > > > Side note - when doing the usual allmodconfig builds with gcc-12 and
> > > > clang before sending them out, for the latter I see this warning being
> > > > spewed with clang-15:
> > > >
> > > > drivers/media/i2c/m5mols/m5mols.o: warning: objtool: m5mols_set_fmt() falls through to next function m5mols_get_frame_desc()
> > > >
> > > > Obviously not related to my changes, but mentioning it in case it has
> > > > been missed as I know you love squeaky clean builds :-). Doesn't happen
> > > > with clang-14.
> > >
> > > Hmm. I have clang-15 too, but I do the allmodconfig builds with gcc,
> > > and only my own "normal config" builds with clang.
> > >
> > > So I don't see this particular issue and my builds are still squeaky clean.
> > >
> > > That said, when I explicitly try that allmodconfig thing with clang, I
> > > can see it too. And the reason seems to be something we've seen
> > > before: UBSAN functions being considered non-return by clang, so clang
> > > generates code like this:
> > >
> > >    ....
> > > .LBB24_3:
> > >         callq   __sanitizer_cov_trace_pc@PLT
> > >         movl    $2, %esi
> > >         movq    $.L__unnamed_3, %rdi
> > >         callq   __ubsan_handle_out_of_bounds
> > > .Lfunc_end24:
> > >         .size   m5mols_set_fmt, .Lfunc_end24-m5mols_set_fmt
> > >
> > > ie the last thing in that m5mols_set_fmt() function is a call to
> > > __ubsan_handle_out_of_bounds, and then it "falls through" to the next
> > > function.
> > >
> > > And yes, I absolutely *detest* how clang does that. Not only does it
> > > cause objtool sanity checking issues, it fundamentally means that we
> > > can never treat UBSAN warnings as warnings. They are always fatal.
>
> 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*

I think no-return is a red herring (or rather, I don't think noreturn
is at play here at all). Looking at the IR, I don't see anything that
indicated anything was deduced to be noreturn.

It looks like this is coming from the loop in __find_restype() being
fully unrolled.

On the initial iteration, `type` == `M5MOLS_RESTYPE_MONITOR` == 0.
`m5mols_default_ffmt` is a 2 element array.
If we don't match we loop again, `type` == `M5MOLS_RESTYPE_CAPTURE` == 1.
`SIZE_DEFAULT_FFMT` == ARRAY_SIZE(m5mols_default_ffmt) == 2, so we loop again.
`type` == 2, accessing m5mols_default_ffmt out of bounds.

Perhaps this code meant to simply use a for loop rather than do-while?
(Or pre-increment rather than post increment for the do-while)?

```
diff --git a/drivers/media/i2c/m5mols/m5mols_core.c
b/drivers/media/i2c/m5mols/m5mols_core.c
index 2b01873ba0db..603b1036127e 100644
--- a/drivers/media/i2c/m5mols/m5mols_core.c
+++ b/drivers/media/i2c/m5mols/m5mols_core.c
@@ -485,10 +485,9 @@ static enum m5mols_restype __find_restype(u32 code)
 {
        enum m5mols_restype type = M5MOLS_RESTYPE_MONITOR;

-       do {
+       for (; type != M5MOLS_RESTYPE_MAX; ++type)
                if (code == m5mols_default_ffmt[type].code)
                        return type;
-       } while (type++ != SIZE_DEFAULT_FFMT);

        return 0;
 }
```

>
> > > But I suspect we need to disable UBSAN for clang, because clang gets
> > > this so *horribly* wrong.

I think Linus is thinking about,
commit e5d523f1ae8f ("ubsan: disable UBSAN_DIV_ZERO for clang")

I can see the loop unroller inserting a branch on "poison" which is a
magic value in LLVM related to but distinct from "undef".  That gets
replaced with an "unreachable" instruction.
I wonder if we can change loop unroller to not insert branch on poison
when the sanitizers are enabled, or freeze poison.
https://llvm.org/devmtg/2020-09/slides/Lee-UndefPoison.pdf

Maybe Linus has thoughts he can share on this thread:
https://github.com/llvm/llvm-project/issues/56289?

Finally, there's always `-mllvm -trap-unreachable` though that's a
last resort kind of thing; `-mllvm` flags need to be passed to the
linker for LTO, and compiler for non-LTO and LTO.  I do think in this
case that the fallthough is bringing to our attention an issue in the
source.

>
> Which is to say, it normally gets it right, but there are some instances
> where things go weird. If it was horribly wrong, there would be a LOT
> more objtool warnings. :)
>
> 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.
>
> --
> Kees Cook



--
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists