linux-kernel - Re: [REGRESSION][BISECTED] erroneous buffer overflow detected in bch2_xattr

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: <20241017185552.GA2263054@thelio-3990X>
Date: Thu, 17 Oct 2024 11:55:52 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Jan Hendrik Farr <kernel@...rr.cc>, Bill Wendling <morbo@...gle.com>,
	Kees Cook <kees@...nel.org>,
	Thorsten Blum <thorsten.blum@...lux.com>, kent.overstreet@...ux.dev,
	regressions@...ts.linux.dev, linux-bcachefs@...r.kernel.org,
	linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org,
	ardb@...nel.org, ojeda@...nel.org
Subject: Re: [REGRESSION][BISECTED] erroneous buffer overflow detected in
 bch2_xattr_validate

Hi Miguel,

On Thu, Oct 17, 2024 at 07:39:43PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 17, 2024 at 6:55 PM Nathan Chancellor <nathan@...nel.org> wrote:
> >
> > Should this include a Fixes tag to give the stable folks a hint about
> > how far back this should go? Maybe
> >
> > Fixes: c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and identifier expansion")
> 
> Yeah, I am not sure -- it does not really fix that commit, but if it
> helps the stable team...

The most "correct" Fixes tag would appear to be the one that first
introduced __counted_by itself (dd06e72e68bc) but __counted_by can never
be used at that original change because the test used __element_count__
as the attribute name, which never shipped in any compiler. So I would
argue that this change really does fix c8248faf3ca2 because that is the
point in time that needs this fix.

> > compiler_attributes.h is intended to be free from compiler and version
> > checks, so adding a version check means that __counted_by() needs to be
> 
> Yeah, ideally we should avoid that since the goal was to have a file
> with the straightforward ones.
> 
> Though if we do go for `CC_HAS_*`, I guess it would be simple enough
> too, i.e. similar to `has_attribute` (but on our side), but it also
> loses the simplicity of knowing those do not have arbitrarily complex
> conditions which `CC_HAS_*` could hide.

Yeah, I think the way compiler_attributes.h has operated so far with
regards to tests and such is working fine so far, no real need to switch
things up yet.

> > moved into compiler_types.h. This might be a good opportunity to
> > introduce something like CC_HAS_COUNTED_BY in Kconfig, so that we can
> > keep the checks unified (since there are already multiple places that
> > want to know about __counted_by support for the sake of testing) and
> > adjust versions like this easily in the future if something else comes
> > up, especially since __counted_by() is not available in a released GCC
> > version yet.
> 
> Sounds good to me (even if we did the unification somewhere else).
> Using `CLANG_VERSION` looks better too.
> 
> > +config CC_HAS_COUNTED_BY
> > +       def_bool $(success,echo 'struct flex { int count; int array[] __attribute__((__counted_by__(count))); };' | $(CC) $(CLANG_FLAGS) -x c - -c -o /dev/null -Werror)
> 
> I am probably missing some context, but what is the reason for the
> build test? i.e. is there a reason we cannot test the GCC version too?

The only reason I generally do build tests plus compiler version checks
is due to certain vendor versions of clang, such as Android, which may
hijack the patch version and appear newer than they actually are.
However, now that I think about it, LLVM moving to GCC's minor
versioning scheme of 0 for the development / main branch and 1+ for
released versions should avoid that issue, so it isn't strictly
necessary for that reason. However...

> If the reason it is that it is not released, should we change it
> later?

This is a good point, as technically to allow use of __counted_by with
GCC with a version check, it would need to be 150000, which would
potentially break GCC versions between the 15 version bump and landing
__counted_by support without the feature check. We could also just do
150100 to be simple about it but I am not sure that is worth doing,
since I believe it is important that we support using __counted_by with
prerelease GCC. We want to make sure that this attribute gets decent
testing coverage while in development.

We could ship this with a comment to simplify the check when GCC 15.1.0
is released, since this is a feature very unlikely to be backported to
earlier GCC releases?

Cheers,
Nathan

I guess since we have a hard version check for clang, we might as well
have one for GCC as well.