[PATCH] adfs: bound the F+ directory entry name length

Yuejie Shi <[email protected]>
Newsgroups gmane.linux.kernel.stable,gmane.linux.file-systems,gmane.linux.kernel
Message-ID <[email protected]>
adfs_fplus_getnext() copies a directory entry name into
struct object_info::name, which is a fixed char[ADFS_MAX_NAME_LEN]
(260 bytes) living on adfs_fplus_iterate()'s stack:

	obj->name_len = le32_to_cpu(bde.bigdirobnamelen);

	offset = adfs_fplus_offset(h, le32_to_cpu(h->bigdirentries));
	offset += le32_to_cpu(bde.bigdirobnameptr);

	ret = adfs_dir_copyfrom(obj->name, dir, offset, obj->name_len);

bigdirobnamelen is a raw on-disk __le32 and is not checked against
anything -- not against the 260-byte destination, not against
ADFS_FPLUS_NAME_LEN (255), not even against bigdirnamesize.
adfs_fplus_validate_header() checks bigdirnamelen, bigdirnamesize and
bigdirentries, but nothing validates the per-entry name length.

adfs_dir_copyfrom() does not save us either.  It bounds only the source,
and even that bound is incomplete: the test

	if (index + (remain < len) >= dir->nr_buffers)
		return -EINVAL;

covers at most one extra buffer, while the tail

	memcpy(dst, dir->bhs[index]->b_data + offset, len);

is not capped at the remaining size of that buffer.  So a large
bigdirobnamelen is an out-of-bounds read of the directory buffer_heads
*and* a linear out-of-bounds write of the same, attacker-chosen, 32-bit
length past object_info::name on the kernel stack.

adfs_object_fixup() then makes it slightly worse: it loops over the same
name_len, and with the "ftsuffix" mount option appends four more bytes at
obj->name[obj->name_len++], which overflows even for name_len == 260.

Reject a name length that cannot fit, and while here make
adfs_dir_copyfrom() check the whole requested range against the
directory's buffers and copy through a loop, so no single memcpy can run
off the end of a buffer_head.

Mounting a crafted image requires CAP_SYS_ADMIN in the initial user
namespace -- ADFS is FS_REQUIRES_DEV and not FS_USERNS_MOUNT -- so this
is not reachable by a plain unprivileged local user.  It matters for the
usual untrusted-media paths: automounted removable media, disk images
handed to a privileged mounting agent (container/VM image tooling), and
forensic or CI systems that mount images to inspect them.  After the
mount, the trigger is a single getdents64() on the root directory, i.e.
"ls /mnt".

  losetup /dev/loop0 adfs-w4.img          # F+ image, bigdirobnamelen=300
  mount -t adfs -o ro /dev/loop0 /mnt
  ls -la /mnt

  BUG: KASAN: stack-out-of-bounds in adfs_dir_copyfrom+0xcc/0x150
  Write of size 300 at addr ffff80008ac6799c by task ls/132
   __asan_memcpy+0x54/0xa0
   adfs_dir_copyfrom+0xcc/0x150
   adfs_fplus_getnext+0x200/0x240
   adfs_fplus_iterate+0x144/0x1b8
   adfs_iterate+0x12c/0x2c0
   iterate_dir+0x12c/0x400
   __arm64_sys_getdents64+0xf0/0x230

followed by cascading stack-out-of-bounds reports in adfs_object_fixup()
and filldir64() walking the smashed frame.  300 is only the smallest
value that demonstrates it; the field is 32 bits wide.

Valid F+ directories are unaffected: RISC OS caps an F+ object name at
ADFS_FPLUS_NAME_LEN (255) bytes, which still leaves room for the four
byte ",xyz" filetype suffix inside ADFS_MAX_NAME_LEN, and the reworked
adfs_dir_copyfrom() copies exactly the same bytes as before for any
request that was in bounds.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: [email protected]
Signed-off-by: Yuejie Shi <[email protected]>
---
 fs/adfs/dir.c       | 15 +++++++++------
 fs/adfs/dir_fplus.c |  2 ++
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 11afa9e157aa..44953247acad 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -19,15 +19,20 @@ int adfs_dir_copyfrom(void *dst, struct adfs_dir *dir,
 		      size_t len)
 {
 	struct super_block *sb = dir->sb;
+	size_t size = (size_t)dir->nr_buffers << sb->s_blocksize_bits;
 	unsigned int index, remain;
 
+	if (offset >= size || len > size - offset)
+		return -EINVAL;
+
 	index = offset >> sb->s_blocksize_bits;
 	offset &= sb->s_blocksize - 1;
-	remain = sb->s_blocksize - offset;
-	if (index + (remain < len) >= dir->nr_buffers)
-		return -EINVAL;
 
-	if (remain < len) {
+	while (len) {
+		remain = sb->s_blocksize - offset;
+		if (remain > len)
+			remain = len;
+
 		memcpy(dst, dir->bhs[index]->b_data + offset, remain);
 		dst += remain;
 		len -= remain;
@@ -35,8 +40,6 @@ int adfs_dir_copyfrom(void *dst, struct adfs_dir *dir,
 		offset = 0;
 	}
 
-	memcpy(dst, dir->bhs[index]->b_data + offset, len);
-
 	return 0;
 }
 
diff --git a/fs/adfs/dir_fplus.c b/fs/adfs/dir_fplus.c
index 4a15924014da..517ffcc91429 100644
--- a/fs/adfs/dir_fplus.c
+++ b/fs/adfs/dir_fplus.c
@@ -192,6 +192,8 @@ adfs_fplus_getnext(struct adfs_dir *dir, struct object
 	obj->indaddr  = le32_to_cpu(bde.bigdirindaddr);
 	obj->attr     = le32_to_cpu(bde.bigdirattr);
 	obj->name_len = le32_to_cpu(bde.bigdirobnamelen);
+	if (obj->name_len > ADFS_FPLUS_NAME_LEN)
+		return -EIO;
 
 	offset = adfs_fplus_offset(h, le32_to_cpu(h->bigdirentries));
 	offset += le32_to_cpu(bde.bigdirobnameptr);
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.