Skip to contentSkip to conversation

fs/ntfs3: fix slab-out-of-bounds write in ni_create_attr_list()

PATCH Superseded hewei-gikaku 2 participants 2 messages Jun 10 – Jul 14 NTFS3 filesystem

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fs/ntfs3: fix slab-out-of-bounds write in ni_create_attr_list()

From: HE WEI(ギカク) <[email protected]>

ni_create_attr_list() allocates a fixed buffer of al_aligned(record_size) (== record_size) bytes and then walks every attribute of the primary MFT record, writing one ATTR_LIST_ENTRY per attribute and advancing the cursor by le_size(name_len), with no check against the end of the buffer; the total size is only computed after the loop.

A minimum-size resident attribute occupies SIZEOF_RESIDENT (0x18 = 24) bytes on disk, but an unnamed attribute expands to le_size(0) (0x20 = 32) bytes in the list. Because the number of attributes in a record is not bounded (mi_enum_attr() accepts arbitrarily many equal-type, nameless minimum-size attributes), a crafted record packed with such attributes produces a list larger than record_size and overflows the heap buffer.

This is reachable from a crafted, loop-mounted NTFS image: opening the file and adding an attribute (e.g. via setxattr) drives ntfs_set_ea() -> ni_insert_resident() -> ni_insert_attr() -> ni_ins_attr_ext() -> ni_create_attr_list().

  BUG: KASAN: slab-out-of-bounds in ni_create_attr_list+0xc48/0x1058
  Write of size 4 at addr ffff000008984c00 by task setfattr/345
   ni_create_attr_list+0xc48/0x1058
   ni_ins_attr_ext+0x510/0x7c0
   ni_insert_attr+0x3f8/0x70c
   ni_insert_resident+0xc8/0x3b0
   ntfs_set_ea+0x66c/0xd28
   ntfs_setxattr+0x4d8/0x5b0
   __arm64_sys_setxattr+0xa4/0x124
  Allocated by task 345:
   ni_create_attr_list+0x188/0x1058
  The buggy address belongs to the cache kmalloc-1k of size 1024
  (the write lands at object+1024).

Size the buffer from the actual attributes instead of assuming a single record_size is always enough.

Fixes 4342306f0f0d ("fs/ntfs3: Add file operations and implementation")
---
 fs/ntfs3/frecord.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
fs/ntfs3/frecord.c +16 -4
index 2e901d073fe9..6488d7a415c0 100644
@@ -768,10 +768,23 @@ int ni_create_attr_list(struct ntfs_inode *ni)
 	rs = sbi->record_size;
 
 	/*
-	 * Skip estimating exact memory requirement.
-	 * Looks like one record_size is always enough.
+	 * Compute the exact size of the attribute list.  Each attribute in the
+	 * record yields one ATTR_LIST_ENTRY of le_size(name_len) bytes.  The
+	 * minimum on-disk attribute is SIZEOF_RESIDENT (0x18) bytes, but an
+	 * unnamed one expands to le_size(0) (0x20) here, so a record crafted
+	 * with many such attributes needs more than a single record_size; the
+	 * previous fixed kzalloc(record_size) could therefore be overflowed by
+	 * an attacker-controlled record.
 	 */
-	le = kzalloc(al_aligned(rs), GFP_NOFS);
+	lsize = 0;
+	attr = NULL;
+	while ((attr = mi_enum_attr(ni, &ni->mi, attr)))
+		lsize += le_size(attr->name_len);
+
+	if (!lsize)
+		return -EINVAL;
+
+	le = kzalloc(al_aligned(lsize), GFP_NOFS);
 	if (!le)
 		return -ENOMEM;
 
@@ -781,7 +794,6 @@ int ni_create_attr_list(struct ntfs_inode *ni)
 	attr = NULL;
 	nb = 0;
 	free_b = 0;
-	attr = NULL;
 
 	for (; (attr = mi_enum_attr(ni, &ni->mi, attr)); le = Add2Ptr(le, sz)) {
 		sz = le_size(attr->name_len);
Signature · 1 line
2.43.0
#1 lore raw
Quoted 84 lines · hewei-gikaku

From: HE WEI(ギカク) <[email protected]>

ni_create_attr_list() allocates a fixed buffer of al_aligned(record_size) (== record_size) bytes and then walks every attribute of the primary MFT record, writing one ATTR_LIST_ENTRY per attribute and advancing the cursor by le_size(name_len), with no check against the end of the buffer; the total size is only computed after the loop.

A minimum-size resident attribute occupies SIZEOF_RESIDENT (0x18 = 24) bytes on disk, but an unnamed attribute expands to le_size(0) (0x20 = 32) bytes in the list. Because the number of attributes in a record is not bounded (mi_enum_attr() accepts arbitrarily many equal-type, nameless minimum-size attributes), a crafted record packed with such attributes produces a list larger than record_size and overflows the heap buffer.

This is reachable from a crafted, loop-mounted NTFS image: opening the file and adding an attribute (e.g. via setxattr) drives ntfs_set_ea() -> ni_insert_resident() -> ni_insert_attr() -> ni_ins_attr_ext() -> ni_create_attr_list().

   BUG: KASAN: slab-out-of-bounds in ni_create_attr_list+0xc48/0x1058
   Write of size 4 at addr ffff000008984c00 by task setfattr/345
    ni_create_attr_list+0xc48/0x1058
    ni_ins_attr_ext+0x510/0x7c0
    ni_insert_attr+0x3f8/0x70c
    ni_insert_resident+0xc8/0x3b0
    ntfs_set_ea+0x66c/0xd28
    ntfs_setxattr+0x4d8/0x5b0
    __arm64_sys_setxattr+0xa4/0x124
   Allocated by task 345:
    ni_create_attr_list+0x188/0x1058
   The buggy address belongs to the cache kmalloc-1k of size 1024
   (the write lands at object+1024).

Size the buffer from the actual attributes instead of assuming a single record_size is always enough.

Fixes 4342306f0f0d ("fs/ntfs3: Add file operations and implementation")
---
  fs/ntfs3/frecord.c | 20 ++++++++++++++++----
  1 file changed, 16 insertions(+), 4 deletions(-)
fs/ntfs3/frecord.c +16 -5
index 2e901d073fe9..6488d7a415c0 100644
@@ -768,10 +768,23 @@ int ni_create_attr_list(struct ntfs_inode *ni)
         rs = sbi->record_size;
 
         /*
-        * Skip estimating exact memory requirement.
-        * Looks like one record_size is always enough.
+        * Compute the exact size of the attribute list.  Each attribute in the
+        * record yields one ATTR_LIST_ENTRY of le_size(name_len) bytes.  The
+        * minimum on-disk attribute is SIZEOF_RESIDENT (0x18) bytes, but an
+        * unnamed one expands to le_size(0) (0x20) here, so a record crafted
+        * with many such attributes needs more than a single record_size; the
+        * previous fixed kzalloc(record_size) could therefore be overflowed by
+        * an attacker-controlled record.
          */
-       le = kzalloc(al_aligned(rs), GFP_NOFS);
+       lsize = 0;
+       attr = NULL;
+       while ((attr = mi_enum_attr(ni, &ni->mi, attr)))
+               lsize += le_size(attr->name_len);
+
+       if (!lsize)
+               return -EINVAL;
+
+       le = kzalloc(al_aligned(lsize), GFP_NOFS);
         if (!le)
                 return -ENOMEM;
 
@@ -781,7 +794,6 @@ int ni_create_attr_list(struct ntfs_inode *ni)
         attr = NULL;
         nb = 0;
         free_b = 0;
-       attr = NULL;
 
         for (; (attr = mi_enum_attr(ni, &ni->mi, attr)); le = Add2Ptr(le, sz)) {
                 sz = le_size(attr->name_len);
--
Quoted 1 line

2.43.0

Hello,

Very sorry for the delay.
The patch was applied, thank you.

Regards,
Konstantin