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

[Kees Cook <kees@kernel.org>]

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