On Fri, Mar 17, 2023 at 12:50 PM Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxx/) > > > > On Fri, Mar 17, 2023 at 11:35 AM Linus Torvalds > > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > On Fri, Mar 17, 2023 at 10:16 AM Jens Axboe <axboe@xxxxxxxxx> 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
