[PATCH v1 2/2] fs/squashfs: bound the offset returned by sqfs_dir_offset()

[Pranav Rajendran <pranavkasthuri@gmail.com>]

Commit 57e0bb7bf00d ("fs/squashfs: add sqfs_dir_offset() error checks")
made sqfs_search_dir() reject negative returns from sqfs_dir_offset(),
but the positive range is still unbounded. Both parts of the returned
offset come from the image: 'offset' is a 16-bit inode field used
verbatim, and the matched metadata block index may be the last one in
m_list, in which case the returned block (j + 1) is one past the end
of the directory table.

The callers use the result to index dirs->dir_table[], which
sqfs_read_directory_table() allocates as m_count metadata blocks, and
then memcpy() a directory header out of it. A crafted image can
therefore read up to 64 KiB past the end of that allocation.

Reject an inode offset that cannot address a decompressed metadata
block, and verify that the resulting directory header lies entirely
within the directory table.

The existing 'offset < 0' test is dropped: 'offset' is assigned from
get_unaligned_le16() and so is never negative, meaning the test never
fired. The new upper bound covers what it was meant to catch.

Fixes: c51006130370 ("fs/squashfs: new filesystem")
Signed-off-by: Pranav Rajendran <pranavkasthuri@gmail.com>
---
 fs/squashfs/sqfs_dir.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

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)
-- 
2.50.1 (Apple Git-155)