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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202309141708.C8B61D4D@keescook>
Date:   Thu, 14 Sep 2023 17:20:41 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Kent Overstreet <kent.overstreet@...ux.dev>
Cc:     Stephen Rothwell <sfr@...b.auug.org.au>,
        Linux Next Mailing List <linux-next@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-hardening@...r.kernel.org
Subject: Re: linux-next: Tree for Sep 12 (bcachefs)

On Thu, Sep 14, 2023 at 03:38:07PM -0400, Kent Overstreet wrote:
> On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> > It looks like you just want a type union for the flexible array.
> > This can be done like this:
> > 
> > struct bch_sb_field_journal_seq_blacklist {
> >         struct bch_sb_field     field;
> > 
> > 	union {
> > 		DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
> > 		DECLARE_FLEX_ARRAY(__u64, _data);
> > 	};
> > };
> 
> Eesh, why though?
> 
> Honestly, I'm not a fan of the change to get rid of zero size arrays,
> this seems to be adding a whole lot of macro layering and indirection
> for nothing.

The C standard doesn't help us in that regard, that's true. But we've
been working to get it fixed. For example, there's discussion happening
next week at GNU Cauldron about flexible arrays in unions. It's already
possible, so better to just fix the standard -- real world code needs it
and uses it, as the bcachefs code illustrates. :)

> The only thing a zero size array could possibly be is a flexible array
> member or a marker, why couldn't we have just kept treating zero size
> arrays like flexible array members?

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.

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.

-Kees

-- 
Kees Cook