Merge patch series "fs/squashfs: bounds checks on image-controlled offsets"

Pranav Rajendran <[email protected]> says:

Two out-of-bounds reads reachable from a crafted SquashFS image, found
while auditing fs/squashfs for image-controlled values used as buffer
offsets without validation. Both were reported to the list earlier; these
are the fixes.

Patch 1 covers sqfs_frag_lookup(), where the fragment index is checked
only against a superblock field that is itself part of the image, and
every subsequent access derived from it is unchecked against the buffer
that was actually read.

Patch 2 covers sqfs_dir_offset(). Commit 57e0bb7bf00d ("fs/squashfs: add
sqfs_dir_offset() error checks") addressed the negative return value; the
positive range is still unbounded, and the callers use it to index the
directory table.

Neither patch changes behaviour for well-formed images: the rejected
cases all describe inodes that reference data outside the tables the
superblock declares.

checkpatch-clean, builds for sandbox with no new warnings at W=1, and
test_sqfs_ls and test_sqfs_load both pass against images generated by
mksquashfs 4.6.1 (the default plus the three lzo fragment variants).

Review of the exact bounds is welcome, in particular whether patch 2 is
too strict in rejecting a directory header that would start within the
last SQFS_DIR_HEADER_SIZE bytes of the directory table.

Link: https://lore.kernel.org/r/[email protected]

2 files changed, 84 insertions, 8 deletions

diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
index 55fbe1bcc1a..cd88923521e 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -109,6 +109,7 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	unsigned char *metadata_buffer, *metadata, *table;
 	struct squashfs_fragment_block_entry *entries;
 	struct squashfs_super_block *sblk = ctxt.sblk;
+	size_t table_size, metadata_size, valid_len;
 	unsigned long dest_len;
 	int block, offset, ret;
 	u16 header;
@@ -133,7 +134,12 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	start /= ctxt.cur_dev->blksz;
 
 	/* Allocate a proper sized buffer to store the fragment index table */
-	table = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
+	if (__builtin_mul_overflow(n_blks, ctxt.cur_dev->blksz, &table_size)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	table = malloc_cache_aligned(table_size);
 	if (!table) {
 		ret = -ENOMEM;
 		goto out;
@@ -148,6 +154,16 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	offset = SQFS_FRAGMENT_INDEX_OFFSET(inode_fragment_index);
 
 	/*
+	 * 'inode_fragment_index' is only checked against sblk->fragments, which
+	 * is itself read from the image, so the resulting index may point past
+	 * the fragment index table that was actually read from the device.
+	 */
+	if (table_offset + ((u64)block + 1) * sizeof(u64) > table_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
 	 * Get the start offset of the metadata block that contains the right
 	 * fragment block entry
 	 */
@@ -158,7 +174,13 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block),
 				  sblk->fragment_table_start, &table_offset);
 
-	metadata_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
+	if (__builtin_mul_overflow(n_blks, ctxt.cur_dev->blksz,
+				   &metadata_size)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	metadata_buffer = malloc_cache_aligned(metadata_size);
 	if (!metadata_buffer) {
 		ret = -ENOMEM;
 		goto out;
@@ -170,6 +192,11 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 	}
 
 	/* Every metadata block starts with a 16-bit header */
+	if (table_offset + SQFS_HEADER_SIZE > metadata_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	header = get_unaligned_le16(metadata_buffer + table_offset);
 	metadata = metadata_buffer + table_offset + SQFS_HEADER_SIZE;
 
@@ -183,6 +210,16 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 		goto out;
 	}
 
+	/*
+	 * The metadata block's payload is read straight out of
+	 * 'metadata_buffer', so it has to fit in what was read from the device.
+	 */
+	if (table_offset + SQFS_HEADER_SIZE + SQFS_METADATA_SIZE(header) >
+	    metadata_size) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	entries = malloc(SQFS_METADATA_BLOCK_SIZE);
 	if (!entries) {
 		ret = -ENOMEM;
@@ -198,8 +235,17 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
 			ret = -EINVAL;
 			goto out;
 		}
+
+		valid_len = dest_len;
 	} else {
 		memcpy(entries, metadata, SQFS_METADATA_SIZE(header));
+		valid_len = SQFS_METADATA_SIZE(header);
+	}
+
+	/* Only the part of 'entries' that was actually filled in is usable */
+	if (((u64)offset + 1) * sizeof(*entries) > valid_len) {
+		ret = -EINVAL;
+		goto out;
 	}
 
 	*e = entries[offset];
diff --git a/fs/squashfs/sqfs_dir.c b/fs/squashfs/sqfs_dir.c
index ed83c90682f..3908d1380b3 100644
--- a/fs/squashfs/sqfs_dir.c
+++ b/fs/squashfs/sqfs_dir.c
@@ -32,6 +32,7 @@ int sqfs_dir_offset(void *dir_i, u32 *m_list, int m_count)
 	struct squashfs_base_inode *base = dir_i;
 	struct squashfs_ldir_inode *ldir;
 	struct squashfs_dir_inode *dir;
+	u64 table_size, res;
 	u32 start_block;
 	int j, offset;
 
@@ -51,20 +52,49 @@ int sqfs_dir_offset(void *dir_i, u32 *m_list, int m_count)
 		return -EINVAL;
 	}
 
-	if (offset < 0)
+	/*
+	 * 'offset' is an offset into a decompressed metadata block, so it can
+	 * never address past the end of one.
+	 */
+	if (offset >= SQFS_METADATA_BLOCK_SIZE)
 		return -EINVAL;
 
+	if (m_count < 1)
+		return -EINVAL;
+
+	/* The caller's directory table holds m_count decompressed blocks. */
+	table_size = (u64)m_count * SQFS_METADATA_BLOCK_SIZE;
+
 	for (j = 0; j < m_count; j++) {
 		if (m_list[j] == start_block)
-			return (++j * SQFS_METADATA_BLOCK_SIZE) + offset;
+			break;
 	}
 
-	if (start_block == 0)
-		return offset;
+	if (j < m_count) {
+		/*
+		 * m_list[j] is the position of the metadata block following
+		 * block j, so a match means the directory starts in block
+		 * j + 1.
+		 */
+		res = (u64)(j + 1) * SQFS_METADATA_BLOCK_SIZE + offset;
+	} else if (start_block == 0) {
+		res = offset;
+	} else {
+		printf("Error: invalid inode reference to directory table.\n");
+		return -EINVAL;
+	}
 
-	printf("Error: invalid inode reference to directory table.\n");
+	/*
+	 * Callers use the return value to index the directory table and read a
+	 * directory header from it, so the whole header must lie inside the
+	 * table.
+	 */
+	if (res + SQFS_DIR_HEADER_SIZE > table_size) {
+		printf("Error: inode points past the end of the directory table.\n");
+		return -EINVAL;
+	}
 
-	return -EINVAL;
+	return res;
 }
 
 bool sqfs_is_empty_dir(void *dir_i)