Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Wed, 20 Sep 2023 08:21:21 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Kent Overstreet <kent.overstreet@linux.dev>
Cc:     Stephen Rothwell <sfr@canb.auug.org.au>,
        Linux Next Mailing List <linux-next@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-hardening@vger.kernel.org
Subject: Re: linux-next: Tree for Sep 12 (bcachefs)
Message-ID: <202309200813.C46E52F4@keescook>
References: <20230912152645.0868a96a@canb.auug.org.au>
 <202309131803.6A3C1D05A@keescook>
 <20230914193807.ozcmylp6n6dsqkbi@moria.home.lan>
 <202309141708.C8B61D4D@keescook>
 <20230919212318.6kr772hz3m5dsyck@moria.home.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230919212318.6kr772hz3m5dsyck@moria.home.lan>
Precedence: bulk

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