linux-ext4 - [PATCH] ext2simg: Fix off-by-one errors causing corruption

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>] [day] [month] [year] [list]

Message-ID: <20211215124452.2934059-1-clemens.lang@bmw.de>
Date:   Wed, 15 Dec 2021 13:44:52 +0100
From:   Clemens Lang <clemens.lang@....de>
To:     <linux-ext4@...r.kernel.org>, Theodore Ts'o <tytso@....edu>
CC:     Mikko Rapeli <mikko.rapeli@....de>,
        Clemens Lang <clemens.lang@....de>
Subject: [PATCH] ext2simg: Fix off-by-one errors causing corruption

If your filesystem is contiguously used from block 0 to block 524286
with a 4096 byte block size, block 524286 did not end up in the sparse
image and was thus restored as zeros when unsparsing.

Additionally, if the last block of the image is used, this block also
ended up being 0:

$> dd if=/dev/zero of=test.ext4 bs=4096 count=10240
$> mkfs.ext4 test.ext4
$> mkdir test
$> sudo mount -t ext4 -o loop test.ext4 test
$> sudo dd if=/dev/urandom of=test/contents
$> sudo umount test
$> ext2simg test.ext4 test.ext4.simg
$> simg2img test.ext4.simg test.ext4.restored
$> vimdiff <(debugfs -R 'bd 10239' test.ext4) <(debugfs -R 'bd 10239' test.ext4.restored)

This seems to have happened because it was not clear that add_chunk()
treats the given end block as exclusive, since the different invocations
of add_chunk() seem to make different assumptions on whether the end
block is included in the sparse image or not. Add documentation to both
the add_chunk function as well as the invocations to clarify this.

This may or may not be the same issue that was reported a few years ago
in https://www.spinics.net/lists/linux-ext4/msg58483.html.

Signed-off-by: Clemens Lang <clemens.lang@....de>
---
 contrib/android/ext2simg.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/contrib/android/ext2simg.c b/contrib/android/ext2simg.c
index 017e16ff..daebb8a8 100644
--- a/contrib/android/ext2simg.c
+++ b/contrib/android/ext2simg.c
@@ -63,6 +63,9 @@ static struct buf_item {
 	void		    *buf[0];
 } *buf_list;
 
+/* Add the blocks [chunk_start, chunk_end) in the filesystem specificed by fs
+ * to the sparse file s. Note that the block at chunk_end is NOT included in
+ * the output. */
 static void add_chunk(ext2_filsys fs, struct sparse_file *s, blk_t chunk_start, blk_t chunk_end)
 {
 	int retval;
@@ -146,16 +149,24 @@ static struct sparse_file *ext_to_sparse(const char *in_file)
 			if (chunk_start == -1) {
 				chunk_start = cur_blk;
 			} else if (cur_blk - chunk_start + 1 == max_blk_per_chunk) {
-				add_chunk(fs, s, chunk_start, cur_blk);
+				/* cur_blk is used, so we must add [chunk_start, cur_blk], i.e.
+				 * pass cur_blk + 1 here. */
+				add_chunk(fs, s, chunk_start, cur_blk + 1);
 				chunk_start = -1;
 			}
 		} else if (chunk_start != -1) {
+			/* cur_blk is unused, so add [chunk_start, cur_blk) */
 			add_chunk(fs, s, chunk_start, cur_blk);
 			chunk_start = -1;
 		}
 	}
-	if (chunk_start != -1)
-		add_chunk(fs, s, chunk_start, cur_blk - 1);
+	if (chunk_start != -1) {
+		/* (cur_blk - 1) must be used, otherwise the code for unused blocks
+		 * above would have processed this. We must include the block, but the
+		 * upper boundary for add_chunk is exclusive, so to add [chunk_start,
+		 * cur_blk - 1] to the sparse image, pass cur_blk. */
+		add_chunk(fs, s, chunk_start, cur_blk);
+	}
 
 	ext2fs_free(fs);
 	return s;
-- 
2.25.1