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
