[PATCH] adfs: validate bigdirobnamelen in adfs_fplus_getnext()

[PATCH] adfs: validate bigdirobnamelen in adfs_fplus_getnext()

[Hui Peng]

adfs_fplus_getnext() reads the directory entry name length straight from
the on-disk F+ big directory entry:

	obj->name_len = le32_to_cpu(bde.bigdirobnamelen);
	...
	ret = adfs_dir_copyfrom(obj->name, dir, offset, obj->name_len);

obj->name is a fixed size array of ADFS_MAX_NAME_LEN (260) bytes inside
the on-stack struct object_info of adfs_fplus_iterate(), but
bigdirobnamelen is fully attacker controlled and never validated.

Mounting a crafted ADFS image whose big directory entry declares
bigdirobnamelen = 264 and then calling getdents64() on the directory
makes adfs_dir_copyfrom() write 264 bytes into the 260 byte obj->name,
smashing the stack frame of adfs_fplus_iterate(), and the subsequent
dir_emit(ctx, obj.name, obj.name_len, ...) reads the same out of bounds
range again in filldir64(). adfs_object_fixup() may then append up to
four more bytes for the ",xyz" filetype suffix, extending the overflow.

Reject entries whose name length exceeds ADFS_FPLUS_NAME_LEN (255).
That is the maximum the F+ format can represent, and it also leaves
room for the four byte filetype suffix appended by adfs_object_fixup()
(255 + 4 = 259 <= ADFS_MAX_NAME_LEN).

Reproduced on Linux 7.3.0-rc3 (5dd1818b15d9) with KASAN by mounting a
64 KiB ADFS image (loop, MS_RDONLY) containing a single F+ directory
entry with bigdirobnamelen = 264 and calling readdir() on the mount
point:

 ==================================================================
 BUG: KASAN: stack-out-of-bounds in memchr+0x82/0xb0
 Read of size 1 at addr ffff88810099fcac by task init/1

 CPU: 3 UID: 0 PID: 1 Comm: init Tainted: G    B D    7.3.0-rc3-g5dd1818b15d9 #1 PREEMPT(lazy)
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 Call Trace:
  <TASK>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  memchr+0x82/0xb0
  filldir64+0x56/0x5a0
  adfs_fplus_iterate+0x1a6/0x2c0
  adfs_iterate+0x1bf/0x4f0
  iterate_dir+0x1c1/0x560
  __x64_sys_getdents64+0x13a/0x260
  do_syscall_64+0xda/0x4b0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 The buggy address belongs to stack of task init/1
  and is located at offset 380 in frame:
  adfs_fplus_iterate+0x0/0x2c0

 This frame has 1 object:
  [32, 320) 'obj'
 ==================================================================

A matching splat is also produced from adfs_object_fixup+0x3f5/0x480
for the write side of the overflow.

With the check in place the same image is rejected with -EIO and no
KASAN splat is produced.

Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
No Fixes: tag: the missing bound check has been present since F+ big
directory support was introduced in fs/adfs/dir_fplus.c, predating the
later refactoring of that file, so there is no single commit to point at.

Found and verified with a QEMU + KASAN reproducer (crafted ADFS image
mounted over loop); the fix was rebuilt and re-run against the same
reproducer, which then reports no KASAN splat.

 fs/adfs/dir_fplus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/adfs/dir_fplus.c b/fs/adfs/dir_fplus.c
index 4a1592401..517ffcc91 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_info *obj)
 	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);
-- 
2.43.0

[PATCH v2] adfs: validate bigdirobnamelen in adfs_fplus_getnext()

[Hui Peng]

adfs_fplus_getnext() reads the directory entry name length straight from
the on-disk F+ big directory entry:

	obj->name_len = le32_to_cpu(bde.bigdirobnamelen);
	...
	ret = adfs_dir_copyfrom(obj->name, dir, offset, obj->name_len);

obj->name is a fixed size array of ADFS_MAX_NAME_LEN (260) bytes inside
the on-stack struct object_info of adfs_fplus_iterate(), but
bigdirobnamelen is fully attacker controlled and never validated.

Mounting a crafted ADFS image whose big directory entry declares
bigdirobnamelen = 264 and then calling getdents64() on the directory
makes adfs_dir_copyfrom() write 264 bytes into the 260 byte obj->name,
smashing the stack frame of adfs_fplus_iterate(), and the subsequent
dir_emit(ctx, obj.name, obj.name_len, ...) reads the same out of bounds
range again in filldir64(). adfs_object_fixup() may then append up to
four more bytes for the ",xyz" filetype suffix, extending the overflow.

Reject entries whose name length exceeds ADFS_FPLUS_NAME_LEN (255).
That is the maximum the F+ format can represent, and it also leaves
room for the four byte filetype suffix appended by adfs_object_fixup()
(255 + 4 = 259 <= ADFS_MAX_NAME_LEN).

Reproduced on Linux 7.3.0-rc3 (5dd1818b15d9) with KASAN by mounting a
64 KiB ADFS image (loop, MS_RDONLY) containing a single F+ directory
entry with bigdirobnamelen = 264 and calling readdir() on the mount
point:

 ==================================================================
 BUG: KASAN: stack-out-of-bounds in memchr+0x82/0xb0
 Read of size 1 at addr ffff88810099fcac by task init/1

 CPU: 3 UID: 0 PID: 1 Comm: init Tainted: G    B D    7.3.0-rc3-g5dd1818b15d9 #1 PREEMPT(lazy)
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 Call Trace:
  <TASK>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  memchr+0x82/0xb0
  filldir64+0x56/0x5a0
  adfs_fplus_iterate+0x1a6/0x2c0
  adfs_iterate+0x1bf/0x4f0
  iterate_dir+0x1c1/0x560
  __x64_sys_getdents64+0x13a/0x260
  do_syscall_64+0xda/0x4b0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 The buggy address belongs to stack of task init/1
  and is located at offset 380 in frame:
  adfs_fplus_iterate+0x0/0x2c0

 This frame has 1 object:
  [32, 320) 'obj'
 ==================================================================

A matching splat is also produced from adfs_object_fixup+0x3f5/0x480
for the write side of the overflow.

With the check in place the same image is rejected with -EIO and no
KASAN splat is produced.

Fixes: da23ef0549d4 ("adfs: add hexadecimal filetype suffix option")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
v2: Add a Fixes: tag. v1 claimed there was no single commit to point
at; that was wrong, so here is the reasoning.

The missing validation itself does date back to the initial git import,
but it was not exploitable then. Before da23ef0549d4, struct
object_info::name_len was an unsigned char and ADFS_MAX_NAME_LEN was
256, so truncating the attacker-controlled 32-bit bigdirobnamelen to
0..255 clamped the copy to within obj->name. da23ef0549d4 widened
name_len to unsigned int (to make room for the ",xyz" filetype suffix)
and grew the buffer only to 256 + 4. That removed the implicit &0xff
clamp, so the full 32-bit on-disk value now reaches
adfs_dir_copyfrom(), which bounds only the source and not the
destination. That is the commit that made the overflow reachable.

For completeness, git blame on the copy points at a317120bf7f8
("fs/adfs: dir: add generic copy functions"), but that commit only
replaces dir_memcpy() with adfs_dir_copyfrom() - same destination, same
unvalidated length - so it is not the right tag.

Backporting: the surrounding context (obj->indaddr, adfs_fplus_offset())
only exists from v5.6, so this will not apply verbatim to older trees.
The added check itself is unchanged; a pre-v5.6 backport just needs the
context adjusted.

ADFS_FPLUS_NAME_LEN (255) rather than ADFS_MAX_NAME_LEN (260) is the
right bound: 255 is the on-disk format maximum and what super.c reports
as s_namelen, and it leaves room for the up-to-4-byte suffix that
adfs_object_fixup() appends (255 + 4 = 259 <= 260).

Found and verified with a QEMU + KASAN reproducer (crafted ADFS image
mounted over loop); the fix was rebuilt and re-run against the same
reproducer, which then reports no KASAN splat.

 fs/adfs/dir_fplus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/adfs/dir_fplus.c b/fs/adfs/dir_fplus.c
index 4a1592401..517ffcc91 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_info *obj)
 	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);
-- 
2.43.0