[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202410040958.C19D3B9E48@keescook>
Date: Fri, 4 Oct 2024 10:13:20 -0700
From: Kees Cook <kees@...nel.org>
To: Jan Hendrik Farr <kernel@...rr.cc>
Cc: 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, morbo@...gle.com
Subject: Re: [REGRESSION][BISECTED] erroneous buffer overflow detected in
bch2_xattr_validate
On Thu, Oct 03, 2024 at 11:48:18PM +0200, Jan Hendrik Farr wrote:
> On 03 14:28:01, Kees Cook wrote:
> > On Thu, Oct 03, 2024 at 05:17:08PM +0200, Jan Hendrik Farr wrote:
> > > gcc currently says that the __bdos of struct containing a flexible array
> > > member is:
> > >
> > > sizeof(<whole struct>) + sizeof(<flexible array element>) * <count>
> > >
> > > clang however does the following:
> > >
> > > max(sizeof(<whole struct>), offsetof(<flexible array member>) + sizeof(<flexible array element>) * <count>)
> >
> > Clang's calculation seems very wrong. I would expect it to match GCC's.
> >
>
> I was on the very same train of thought, but I have since changed my
> mind a bit. A struct containing a flexible array member can be allocated in
> two ways:
>
> (1):
>
> struct posix_acl *acl = malloc(sizeof(struct posix_acl) + sizeof(struct posix_acl_entry) * 1);
> acl.a_count = 1;
>
> or (2):
>
> struct posix_acl *acl = malloc(offsetof(struct posix_acl, a_entries) + sizeof(struct posix_acl_entry) * 1);
> acl.a_count = 1;
>
> Both are valid ways to allocate it. __bdos does not know which of these
> methods was used to allocate the struct whose size it has to determine,
> so it's giving the lower bound that doesn't include the (potential)
> padding at the end.
I want to separate several easily confused issues. Instead of just
saying __bdos, let's clearly refer to what calculation within bdos is
being used. There are 3 choices currently:
- alloc_size attribute
- counted_by attribute
- fallback to __bos (which is similar to sizeof(), except that FAMs are 0 sized)
Additionally there are (for all intents and purposes) 2 size
determinations to be made by __bos and __bdos, via argument 2:
- containing object size (type 0) ("maximum size")
- specific object size (type 1) ("minimum size")
For example, consider:
struct posix_acl *acl = malloc(1024);
acl->a_count = 1;
what should these return:
__bos(acl, 0)
__bos(acl, 1)
__bdos(acl, 0)
__bdos(acl, 1)
__bos(acl->a_entries, 0)
__bos(acl->a_entries, 1)
__bdos(acl->a_entries, 0)
__bdos(acl->a_entries, 1)
> So it comes down to false positives vs false negatives...
> More details here:
> https://github.com/llvm/llvm-project/pull/111015
>
> Clangs current behavior would essentially force kernel code to always
> assume option (2) is used. So
>
> struct posix_acl *
> posix_acl_clone(const struct posix_acl *acl, gfp_t flags)
> {
> struct posix_acl *clone = NULL;
>
> if (acl) {
> int size = sizeof(struct posix_acl) + acl->a_count *
> sizeof(struct posix_acl_entry);
> clone = kmemdup(acl, size, flags);
> if (clone)
> refcount_set(&clone->a_refcount, 1);
> }
> return clone;
> }
> EXPORT_SYMBOL_GPL(posix_acl_clone);
>
> from linux/fs/posix_acl.c would have to turn into something like:
>
> struct posix_acl *
> posix_acl_clone(const struct posix_acl *acl, gfp_t flags)
> {
> struct posix_acl *clone = NULL;
>
> if (acl) {
> int size = offsetof(struct posix_acl, a_entries) + acl->a_count *
> sizeof(struct posix_acl_entry);
> clone = kmemdup(acl, size, flags);
> if (clone)
> refcount_set(&clone->a_refcount, 1);
> }
> return clone;
> }
> EXPORT_SYMBOL_GPL(posix_acl_clone);
>
> Which is actually safer, because can you actually be sure this posix_acl
> wasn't allocated using method (2)?
First, this should not be using an open coded calculation at all; it
should use the struct_size() macro.
Secondly, if we want to change struct_size(), then we must (via
allmodconfig builds) determine all the places in the kernel
where the calculated size changes, and audit those for safety.
Right now, struct_size() over-estimates in the face of padding.
We're already moving the kernel toward not even calling struct_size()
externally from the allocation, and instead using the it within the
allocation macros themselves:
https://lore.kernel.org/lkml/20240822231324.make.666-kees@kernel.org/
> After looking at the assembly produced by gcc more, it actually looks
> like it's using the allocation size if it's known in the current context
> (for example if the struct was just malloced in the same function)
> and otherwise returns INT_MAX for the __bdos of a struct containing a
> flexible array member. It's only returning the size based on the
> __counted_by attribute of you ask it for the __bdos of the flexible
> array member itself.
Here is my test case for all the corner cases we've found so far:
https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c
I'd prefer we add cases there so we can all be talking about the same
things. :)
-Kees
--
Kees Cook
Powered by blists - more mailing lists