Re: [REGRESSION][BISECTED] erroneous buffer overflow detected in bch2_xattr_validate

[Bill Wendling]

On Fri, Oct 4, 2024 at 10:13 AM Kees Cook <kees@xxxxxxxxxx> wrote:
> 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.
>
Slightly off topic: while I was looking at the definition for
struct_size, I noticed this:

#define flex_array_size(p, member, count)                               \
        __builtin_choose_expr(__is_constexpr(count),                    \
                (count) * sizeof(*(p)->member) +
__must_be_array((p)->member),  \
                size_mul(count, sizeof(*(p)->member) +
__must_be_array((p)->member)))

In particular the 'size_mul' line. I realize that '__must_be_array'
will return a '0' or static compiler error, but it seems inconsistent
that it's included as part of 'size_mul'. Maybe something like this
instead (notice the parens on the last line)?

#define flex_array_size(p, member, count)                               \
        __builtin_choose_expr(__is_constexpr(count),                    \
                (count) * sizeof(*(p)->member) +
__must_be_array((p)->member),  \
                size_mul(count, sizeof(*(p)->member)) +
__must_be_array((p)->member))


> 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@xxxxxxxxxx/
>
> > 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