linux-ext4 - Re: [PATCH] e2fsck: blk64_t to blk

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: <20131203002503.GA18601@thunk.org>
Date:	Mon, 2 Dec 2013 19:25:03 -0500
From:	Theodore Ts'o <tytso@....edu>
To:	Kit Westneat <kwestneat@....com>
Cc:	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	"Dilger, Andreas" <andreas.dilger@...el.com>
Subject: Re: [PATCH] e2fsck: blk64_t to blk_t truncation

On Mon, Dec 02, 2013 at 03:47:36PM -0500, Kit Westneat wrote:
> 
> We recently ran into an issue where e2fsck was claiming to clear
> deleted/unused inodes, but was not actually doing it. We tracked it
> down to a bad blk64_t to blk_t conversion in pass2 where
> ext2fs_write_dir_block is being called instead of
> ext2fs_write_dir_block3. There is another similar call in
> allocate_dir_block. These appear to be fixed in master as part of
> the move to adding checksums to the end of directory leaf nodes, but
> it's still an issue on maint.
> 
> I ran gcc with -Wconversion and found a number of other places that
> have blk64_t to blk_t truncations, but I'm not sure how many of them
> are actually problems. For example in block_iterate_ind, block_nr is
> blk_t but it seems to be a specific choice, since there is also the
> blk64 variable. Another instance is with the EA refcounting, which
> is all 32-bit. In expand_dir_proc in pass3.c, it uses the 32-bit
> ext2fs_write_dir_block. Against 1.42.7, gcc reports 103 blk64_t to
> blk_t conversions, so this is just a small sample.

We can only reference 32-bit block numbers using the indirect block
scheme, so that's probably OK.  The EA refcounting is definitely a
problem.  As is the usages of expand_dir_proc in e2fsck/pass3.c.
Thanks for pointing that out.  We'll need to do an audit with
-Wconversion.  Unfortunately it is quite noisy, as you've noted.

I took a quick check about whether we could add -Wconversion to the
list of things which "make gcc-wall" would use, but the problem is
that it's quite noisy about things things things such as the bit
swapping and bit operations in bitops.h.

i've expanded your patch to fix up all of the useages of
ext2fs_write_dir_block() in e2fsck and applied it to my maint branch.
Thanks for pointing this out!

			    	     	    - Ted

commit 7bef6d52125ef3f1ef07d9da71a13546f6843c56
Author: Kit Westneat <kwestneat@....com>
Date:   Mon Dec 2 19:11:52 2013 -0500

    e2fsck: use ext2fs_write_dir_block3() instead of ext2fs_write_dir_block()
    
    The use of ext2fs_write_dir_block() meant that attempts to fix
    deleted/unused inodes in a directory would not be fixed for file
    systems with 64-bit block numbers.  (And some random block with the
    high 32-bits cleared would get corrupted.)
    
    Fix a similar problem when expanding directories and when creating the
    lost+found dirctory.
    
    Signed-off-by: Kit Westneat <kwestneat@....com>
    Signed-off-by: "Theodore Ts'o" <tytso@....edu>
    Reviewed-by: Darrick J. Wong <darrick.wong@...cle.com>

diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
index bceadfe..f2ac2dd 100644
--- a/e2fsck/pass2.c
+++ b/e2fsck/pass2.c
@@ -1132,7 +1132,7 @@ out_htree:
 		}
 	}
 	if (dir_modified) {
-		cd->pctx.errcode = ext2fs_write_dir_block(fs, block_nr, buf);
+		cd->pctx.errcode = ext2fs_write_dir_block3(fs, block_nr, buf, 0);
 		if (cd->pctx.errcode) {
 			if (!fix_problem(ctx, PR_2_WRITE_DIRBLOCK,
 					 &cd->pctx))
@@ -1455,7 +1455,7 @@ static int allocate_dir_block(e2fsck_t ctx,
 		return 1;
 	}
 
-	pctx->errcode = ext2fs_write_dir_block(fs, blk, block);
+	pctx->errcode = ext2fs_write_dir_block3(fs, blk, block, 0);
 	ext2fs_free_mem(&block);
 	if (pctx->errcode) {
 		pctx->str = "ext2fs_write_dir_block";
diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
index e358bb2..926f462 100644
--- a/e2fsck/pass3.c
+++ b/e2fsck/pass3.c
@@ -198,9 +198,9 @@ static void check_root(e2fsck_t ctx)
 		return;
 	}
 
-	pctx.errcode = ext2fs_write_dir_block(fs, blk, block);
+	pctx.errcode = ext2fs_write_dir_block3(fs, blk, block, 0);
 	if (pctx.errcode) {
-		pctx.str = "ext2fs_write_dir_block";
+		pctx.str = "ext2fs_write_dir_block3";
 		fix_problem(ctx, PR_3_CREATE_ROOT_ERROR, &pctx);
 		ctx->flags |= E2F_FLAG_ABORT;
 		return;
@@ -444,7 +444,7 @@ ext2_ino_t e2fsck_get_lost_and_found(e2fsck_t ctx, int fix)
 		return 0;
 	}
 
-	retval = ext2fs_write_dir_block(fs, blk, block);
+	retval = ext2fs_write_dir_block3(fs, blk, block, 0);
 	ext2fs_free_mem(&block);
 	if (retval) {
 		pctx.errcode = retval;
@@ -725,7 +725,7 @@ static int expand_dir_proc(ext2_filsys fs,
 			return BLOCK_ABORT;
 		}
 		es->num--;
-		retval = ext2fs_write_dir_block(fs, new_blk, block);
+		retval = ext2fs_write_dir_block3(fs, new_blk, block, 0);
 	} else {
 		retval = ext2fs_get_mem(fs->blocksize, &block);
 		if (retval) {
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html