Re: linux-next: Tree for Sep 12 (bcachefs)

[Kees Cook <keescook@chromium.org>]

On Tue, Sep 19, 2023 at 05:23:18PM -0400, Kent Overstreet wrote:
> On Thu, Sep 14, 2023 at 05:20:41PM -0700, Kees Cook wrote:
> > Because they're ambiguous and then the compiler can't do appropriate
> > bounds checking, compile-time diagnostics, etc. Maybe it's actually zero
> > sized, maybe it's not. Nothing stops them from being in the middle of
> > the structure so if someone accidentally tries to put members after it
> > (which has happened before), we end up with bizarre corruptions, etc,
> > etc. Flexible arrays are unambiguous, and that's why we committed to
> > converting all the fake flex arrays. The compiler does not have to guess
> > (or as has been the case: give up on) figuring out what was intended.
> 
> So it does seem like we need to be able to distinguish between normal
> flex arrays that go at the end of a struct vs. - what should we call
> them, markers? that go in the middle.

As long as markers are just treated as address offsets in an struct, I
don't see a problem with them being 0-length arrays. I personally find
them confusing since whatever follows the marker is usually what I'm
trying to address, so the marker serves no purpose.

In the case of finding the offset to a subset of struct members, we
moved all of those in the kernel to using struct_group() instead. But
again, this was just for removing ambiguity for the compiler's ability
to enforce bounds checking (in this case on the memcpy()-family of
functions).

> 
> > Regardless, I'm just trying to help make sure folks that run with
> > CONFIG_UBSAN_BOUNDS=y (as done in Android, Ubuntu, etc) will be able to
> > use bcachefs without runtime warnings, etc. Indexing through a 0-sized
> > array is going to trip the diagnostic either at runtime or when building
> > with -Warray-bounds.
> 
> I do have CONFIG_UBSAN_BOUNDS=y testing in my own CI, so all the runtime
> errors should be fixed now (some of them with casts, but the casts are
> in helpers that know what they're doing, not scattered around at
> random).

Great! Thank you for chasing them all down. If you also have
CONFIG_FORTIFY_SOURCE=y then that should also be checking all the
strcpy()/memcpy() families too. The only thing that may be a problem in
the future is our effort to enable -Warray-bounds at build time. GCC
still has one false positive[1] remaining, but once that's fixed
(hopefully for GCC 14) the rest of the kernel is (was?) warning-free
(in our local testing where CONFIG_CC_NO_ARRAY_BOUNDS has been disabled).

> 
> So I think we're good for now - I'm going to hold off on more cleanup
> for now unless reports of actual ubsan splats turn up, since I'm getting
> a bit bombarded at the moment :)

Understood! :)

-Kees

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071

-- 
Kees Cook