[PATCH] ntfs: validate $FILE_NAME length before converting the name

[Bryam Vargas]

ntfs_is_extended_system_file() walks a base inode's $FILE_NAME attributes
and, for a name whose parent is $Extend, converts the on-disk name to the
local charset via ntfs_attr_name_get() -> ntfs_ucstonls(). It checks that
the resident value [file_name_attr, +value_length) lies within the
attribute, but never checks that file_name_length (an on-disk u8) fits
inside that value.

ntfs_ucstonls() then reads file_name_length __le16 characters starting at
file_name_attr->file_name (offset 66 in the __packed struct), so a crafted
$FILE_NAME whose value_length covers just the fixed header but whose
file_name_length is 255 drives an out-of-bounds read of up to ~508 bytes
past the resident value -- and, when the MFT record buffer is a tight
kmalloc (mft_record_size equal to a power-of-two slab bucket), past the
allocation itself.

Reject the attribute when file_name_length does not fit within
value_length before the name is read.

Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
---
Reproducer omitted on the public list -- available to the maintainers on
request. This is a crafted-image filesystem bug; posting publicly per the
security@xxxxxxxxxx guidance on the sibling fs/ntfs attribute-list fixes that
such bugs fall outside the kernel security process's threat model.

The over-read was observed under KASAN by mounting a crafted NTFS image
(fs/ntfs built as a module against v7.1-rc5): a $FILE_NAME on a system file in
$Extend with value_length 68 and file_name_length 255 trips, during inode
read, a slab-out-of-bounds read of size 2 in utf16s_to_utf8s() along the chain

  ntfs_ucstonls <- ntfs_attr_name_get <- ntfs_is_extended_system_file <-
  ntfs_read_locked_inode <- ntfs_iget <- ntfs_fill_super

off the kmalloc-1k MFT record buffer (mft_record_size 1024, allocated by
map_mft_record_folio). A benign mkntfs image is clean on the same kernel
(control). With this bound in place the crafted $FILE_NAME is rejected with
-EIO ("Corrupt file name attribute") before the conversion, while a benign
name (value_length == 66 + file_name_length * 2) still passes. The bound is arch-independent: struct
file_name_attr is __packed, so offsetof(.., file_name) == 66 and the check
decides every case identically built -m32/-m64.

The only other reader of file_name_length for name conversion is under
#ifdef DEBUG in fs/ntfs/namei.c (not built in production); it can be guarded
the same way if desired.

The flaw is original to the fs/ntfs driver: ntfs_is_extended_system_file()
landed with the driver itself (linkinjeon/ntfs, v7.1 merge window). A Fixes:
tag against that introducing commit is appropriate -- I'd appreciate you
pinning the exact SHA from your tree, as it is not unambiguously citable from
a mainline clone. checkpatch clean.

 fs/ntfs/inode.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 360bebd1ee3f..d1a79c276502 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -581,6 +581,17 @@ static int ntfs_is_extended_system_file(struct ntfs_attr_search_ctx *ctx)
 		p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
 		if (p2 < (u8 *)attr || p2 > p)
 			goto err_corrupt_attr;
+		/*
+		 * The name is converted below; file_name_length is an on-disk
+		 * u8 that is not otherwise cross-checked, so make sure the name
+		 * fits within the resident value before it is read.
+		 */
+		if (le32_to_cpu(attr->data.resident.value_length) <
+				offsetof(struct file_name_attr, file_name) ||
+		    offsetof(struct file_name_attr, file_name) +
+				file_name_attr->file_name_length * sizeof(__le16) >
+				le32_to_cpu(attr->data.resident.value_length))
+			goto err_corrupt_attr;
 		/* This attribute is ok, but is it in the $Extend directory? */
 		if (MREF_LE(file_name_attr->parent_directory) == FILE_Extend) {
 			unsigned char *s;
--
2.43.0