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: <Zv61dCaxScXuOjZg@archlinux>
Date: Thu, 3 Oct 2024 17:17:08 +0200
From: Jan Hendrik Farr <kernel@...rr.cc>
To: Thorsten Blum <thorsten.blum@...lux.com>
Cc: Kees Cook <kees@...nel.org>, 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 03 15:07:52, Thorsten Blum wrote:
> On 3. Oct 2024, at 13:33, Jan Hendrik Farr <kernel@...rr.cc> wrote:
> >> [...]
> > 
> > This issue is now fixed on the llvm main branch:
> > https://github.com/llvm/llvm-project/commit/882457a2eedbe6d53161b2f78fcf769fc9a93e8a
> 
> Thanks!
> 
> Do you know if it also fixes the different sizes here:
> https://godbolt.org/z/vvK9PE1Yq

I have a patch for clang that changes the behavior to what gcc does and
what the kernel seems to be expecting right now, you can find it below.

I'm not 100% sure what if the gcc or the clang behavior is currently
correct. However, I'm gonna argue that gcc has it correct.

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>)


The kernel assumes the gcc behvaior in places like linux/fs/posix_acl.c:

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);

This is the code that triggers the problem in [1]. The way I see it, this
code should work, as you also allocate struct posix_acl with the same
sizeof(struct posix_acl) + acl->a_count * sizeof(struct posix_acl_entry)
as an argument to malloc (or in-kernel equivalent).

Based on the C standard the size of that object is the size passed to
malloc. See bottom of page 348 [2].


I'll put together another PR to llvm with this fix, just need to
add/change tests.

[1] https://lore.kernel.org/linux-kernel/3D0816D1-0807-4D37-8D5F-3C55CA910FAA@linux.dev/
[2] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf


--
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index d739597de4c8..1d112aededbd 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -919,8 +919,7 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
   //   2) bdos of the whole struct, including the flexible array:
   //
   //     __builtin_dynamic_object_size(p, 1) ==
-  //        max(sizeof(struct s),
-  //            offsetof(struct s, array) + p->count * sizeof(*p->array))
+  //        sizeof(struct s) + p->count * sizeof(*p->array))
   //
   ASTContext &Ctx = getContext();
   const Expr *Base = E->IgnoreParenImpCasts();
@@ -1052,22 +1051,13 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
     // The whole struct is specificed in the __bdos.
     const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);
 
-    // Get the offset of the FAM.
-    llvm::Constant *FAMOffset = ConstantInt::get(ResType, Offset, IsSigned);
-    Value *OffsetAndFAMSize =
-        Builder.CreateAdd(FAMOffset, Res, "", !IsSigned, IsSigned);
 
     // Get the full size of the struct.
     llvm::Constant *SizeofStruct =
         ConstantInt::get(ResType, Layout.getSize().getQuantity(), IsSigned);
 
-    // max(sizeof(struct s),
-    //     offsetof(struct s, array) + p->count * sizeof(*p->array))
-    Res = IsSigned
-              ? Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
-                                              OffsetAndFAMSize, SizeofStruct)
-              : Builder.CreateBinaryIntrinsic(llvm::Intrinsic::umax,
-                                              OffsetAndFAMSize, SizeofStruct);
+    // Add full size of struct and fam size
+    Res = Builder.CreateAdd(SizeofStruct, Res, "", !IsSigned, IsSigned);
   }
 
   // A negative \p IdxInst or \p CountedByInst means that the index lands