ext2simg: fix off-by-one errors causing corruption

The chunk_end parameter to add_chunk() is exclusive, but two callers
incorrectly treat it as inclusive: when the maximum chunk length of
'INT32_MAX - 12' bytes is reached, and when a chunk extends to the very
end of the filesystem.  The result is that the output simg file contains
zeroes for the last block of these chunks instead of the correct data.

A related bug is that the expanded size of the simg file is set to the
filesystem size (in blocks) minus s_first_data_block.  On filesystems
where s_first_data_block != 0, i.e. 1K blocksize filesystems without
bigalloc enabled, this truncates the last block of the filesystem.

Fix these bugs by (a) making add_chunk() take the chunk length and
passing the correct values, and (b) using the filesystem size properly.

Here is a reproducer that shows the last block of the filesystem being
truncated (bsize=1024) and being corrupted with zeroes (bsize=4096):

    for bsize in 1024 4096; do
        rm -f ext4.img
        mkfs.ext4 -b $bsize ext4.img 10000
        mkdir -p mnt
        sudo mount -t ext4 -o loop ext4.img mnt
        sudo cp /dev/urandom mnt/fill
        sudo umount mnt
        ext2simg ext4.img ext4.simg
        simg2img ext4.simg ext4.img.restored
        cmp ext4.img ext4.img.restored
    done

Fixes: db6f320912cf ("AOSP: android: add the ext2simg tool")
Reported-by: Clemens Lang <clemens.lang@bmw.de>
Change-Id: I3b64c4fbffa5821b431f29e99b36168617da7563
Signed-off-by: Eric Biggers <ebiggers@google.com>

1 files changed, 33 insertions, 16 deletions

diff --git a/contrib/android/ext2simg.c b/contrib/android/ext2simg.c
index 017e16ff..9ef54cff 100644
--- a/contrib/android/ext2simg.c
+++ b/contrib/android/ext2simg.c
@@ -63,12 +63,16 @@ static struct buf_item {
 	void		    *buf[0];
 } *buf_list;
 
-static void add_chunk(ext2_filsys fs, struct sparse_file *s, blk_t chunk_start, blk_t chunk_end)
+/*
+ * Add @num_blks blocks, starting at index @chunk_start, of the filesystem @fs
+ * to the sparse file @s.
+ */
+static void add_chunk(ext2_filsys fs, struct sparse_file *s,
+		      blk_t chunk_start, int num_blks)
 {
 	int retval;
-	unsigned int nb_blk = chunk_end - chunk_start;
-	size_t len = nb_blk * fs->blocksize;
-	int64_t offset = (int64_t)chunk_start * (int64_t)fs->blocksize;
+	uint64_t len = (uint64_t)num_blks * fs->blocksize;
+	int64_t offset = (int64_t)chunk_start * fs->blocksize;
 
 	if (params.overwrite_input == false) {
 		if (sparse_file_add_file(s, params.in_file, offset, len, chunk_start) < 0)
@@ -86,9 +90,9 @@ static void add_chunk(ext2_filsys fs, struct sparse_file *s, blk_t chunk_start,
 			buf_list = bi;
 		}
 
-		retval = io_channel_read_blk64(fs->io, chunk_start, nb_blk, bi->buf);
+		retval = io_channel_read_blk64(fs->io, chunk_start, num_blks, bi->buf);
 		if (retval < 0)
-			ext2fs_fatal(retval, "reading block %u - %u", chunk_start, chunk_end);
+			ext2fs_fatal(retval, "reading data from %s", params.in_file);
 
 		if (sparse_file_add_data(s, bi->buf, len, chunk_start) < 0)
 			sparse_fatal("adding data to the sparse file");
@@ -112,7 +116,7 @@ static struct sparse_file *ext_to_sparse(const char *in_file)
 	ext2_filsys fs;
 	struct sparse_file *s;
 	int64_t chunk_start = -1;
-	blk_t first_blk, last_blk, nb_blk, cur_blk;
+	blk_t fs_blks, cur_blk;
 
 	retval = ext2fs_open(in_file, 0, 0, 0, unix_io_manager, &fs);
 	if (retval)
@@ -122,11 +126,9 @@ static struct sparse_file *ext_to_sparse(const char *in_file)
 	if (retval)
 		ext2fs_fatal(retval, "while reading block bitmap of %s", in_file);
 
-	first_blk = ext2fs_get_block_bitmap_start2(fs->block_map);
-	last_blk = ext2fs_get_block_bitmap_end2(fs->block_map);
-	nb_blk = last_blk - first_blk + 1;
+	fs_blks = ext2fs_blocks_count(fs->super);
 
-	s = sparse_file_new(fs->blocksize, (uint64_t)fs->blocksize * (uint64_t)nb_blk);
+	s = sparse_file_new(fs->blocksize, (uint64_t)fs_blks * fs->blocksize);
 	if (!s)
 		sparse_fatal("creating sparse file");
 
@@ -140,22 +142,37 @@ static struct sparse_file *ext_to_sparse(const char *in_file)
 	 */
 	int64_t max_blk_per_chunk = (INT32_MAX - 12) / fs->blocksize;
 
-	/* Iter on the blocks to merge contiguous chunk */
-	for (cur_blk = first_blk; cur_blk <= last_blk; ++cur_blk) {
+	/*
+	 * Iterate through the filesystem's blocks, identifying "chunks" that
+	 * are contiguous ranges of blocks that are in-use by the filesystem.
+	 * Add each chunk to the sparse_file.
+	 */
+	for (cur_blk = ext2fs_get_block_bitmap_start2(fs->block_map);
+	     cur_blk < fs_blks; ++cur_blk) {
 		if (ext2fs_test_block_bitmap2(fs->block_map, cur_blk)) {
+			/*
+			 * @cur_blk is in-use.  Append it to the pending chunk
+			 * if there is one, otherwise start a new chunk.
+			 */
 			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);
+				/*
+				 * Appending @cur_blk to the pending chunk made
+				 * it reach the maximum length, so end it.
+				 */
+				add_chunk(fs, s, chunk_start, max_blk_per_chunk);
 				chunk_start = -1;
 			}
 		} else if (chunk_start != -1) {
-			add_chunk(fs, s, chunk_start, cur_blk);
+			/* @cur_blk is not in-use, so end the pending chunk. */
+			add_chunk(fs, s, chunk_start, cur_blk - chunk_start);
 			chunk_start = -1;
 		}
 	}
+	/* If there's still a pending chunk, end it. */
 	if (chunk_start != -1)
-		add_chunk(fs, s, chunk_start, cur_blk - 1);
+		add_chunk(fs, s, chunk_start, cur_blk - chunk_start);
 
 	ext2fs_free(fs);
 	return s;