linux-ext4 - Re: Minimal configuration for e2fsprogs

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]

Date:	Thu, 28 Jun 2012 12:43:56 +1000
From:	Tony Breeds <tony@...eyournoodle.com>
To:	Theodore Ts'o <tytso@....edu>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: Minimal configuration for e2fsprogs

On Wed, Jun 27, 2012 at 08:54:43AM -0400, Theodore Ts'o wrote:
 
> Well, rb_insert_extent() isn't returning an error; it's returning
> whether or not it needed to insert an extent or not.  And the bigger
> problem is there's no way to return an error all the way up the
> callchain, since it ultimately gets called by functions like
> ext2fs_mark_block_bitmap() which never contemplated that the function
> might fail.

Okay.  I shoudl have read more carefully.
 
> So there really is no way to return an error at this point, and if you
> fail allocating memory, we're kind of doomed anyway.  We could replace
> this with an abort(), but there's really not much else we can do here.
> 
> I suppose, more generally, we could add a new callback which gets
> called on memory allocation failures; although in practice, the place
> where we are most likely to run into trouble is e2fsck, and we already
> have sufficient debugging code there thanks to e2fsck/sigcatcher.c.
> So maybe just using an abort() is the best approach for now.

Okay so I think you want somethign like:
---
diff --git a/e2fsck/sigcatcher.c b/e2fsck/sigcatcher.c
index 10b9328..bd56c3f 100644
--- a/e2fsck/sigcatcher.c
+++ b/e2fsck/sigcatcher.c
@@ -387,6 +387,7 @@ void sigcatcher_setup(void)
 	sigaction(SIGILL, &sa, 0);
 	sigaction(SIGBUS, &sa, 0);
 	sigaction(SIGSEGV, &sa, 0);
+	sigaction(SIGABRT, &sa, 0);
 }	
 
 
diff --git a/lib/ext2fs/blkmap64_rb.c b/lib/ext2fs/blkmap64_rb.c
index e6b7e04..74140ec 100644
--- a/lib/ext2fs/blkmap64_rb.c
+++ b/lib/ext2fs/blkmap64_rb.c
@@ -134,10 +134,8 @@ static void rb_get_new_extent(struct bmap_rb_extent **ext, __u64 start,
 
 	retval = ext2fs_get_mem(sizeof (struct bmap_rb_extent),
 				&new_ext);
-	if (retval) {
-		perror("ext2fs_get_mem");
-		exit(1);
-	}
+	if (retval)
+		abort();
 
 	new_ext->start = start;
 	new_ext->count = count;
---

Adding something that calls itself abort() wont be hard in yaboot.

> So I believe the only place where the dblist.o file is getting dragged
> in is due to the call to ext2fs_free_dblist() in freefs.c.  Can you
> confirm this?

What I did was remove dblist.o from my libext2fs.a and I now get:
/home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(gen_bitmap64.o): In function `ext2fs_alloc_generic_bmap':
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/gen_bitmap64.c:112: undefined reference to `ext2fs_get_num_dirs'

So it looks like dblist.o is being pulled in for ext2fs_get_num_dirs()
in that case a EXT2FS_BMAP64_AUTODIR bmap type has been asked for.

How would you feel about moving ext2fs_get_num_dirs from dblist.c to
blknum.c?

> If so, probably the way to solve this is the via the same trick we do
> with fs->write_bitmaps() --- see how we only call fs->write_bitmaps()
> if it is defined in ext2fs_close2(); this was done precisely to avoid
> dragging in the write_bitmaps code if it's not needed for programs
> which are opening the file system read/only.

I didn't really look at this because of what I discovered above.

Yours Tony

Content of type "application/pgp-signature" skipped