Skip to contentSkip to conversation

fs/ntfs3: validate target_attr in log_replay()

PATCH In review Cen Zhang (Microsoft Security FORGE Labs) 2 participants 2 messages Sep 1 – Sep 2

Subsystems (2)
Stable branch NTFS3 filesystem
lore
Share & tools
lore

Cen Zhang (Microsoft Security FORGE Labs)

author
#0 lore raw

PATCH 1/1 fs/ntfs3: validate target_attr in log_replay()

log_replay() treats lrh->target_attr as a byte offset to an OPEN_ATTR_ENRTY in the open-attribute restart table. A valid offset is the table header size plus an integral number of entries. The redo lookup checks only target_attr < bytes_per_rt(oatbl); its earlier alignment check subtracts the header size without first checking the lower bound, so the subtraction can wrap.

The undo lookup has no table-bound check, and its earlier alignment check is skipped when lcns_follow is zero. Unlike the redo lookup, it also does not reject unallocated entries or NULL entry pointers.

A crafted $LogFile can therefore point into the table header, the middle of an entry, or past the table and trigger an out-of-bounds access.

BUG: KASAN: slab-out-of-bounds in log_replay+0x7d44/0x9d80
  fs/ntfs3/fslog.c:5235 log_replay()
  ntfs_loadlog_and_replay()
  ntfs_fill_super()
  path_mount()

Add rstbl_entry_valid() to require a nonzero entry size and an offset between the table header and end that is aligned to the entry size. Use it before both lookups, and make the undo lookup match the redo lookup by rejecting unallocated entries and NULL entry pointers. Skip targetless records with no LCN work before the lookup so their unused target_attr is not subjected to the new validation.

Fixes b46acd6a6a62 ("fs/ntfs3: Add NTFS journal")
Reported-by Xiang Mei (Microsoft) <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Cen Zhang (Microsoft Security FORGE Labs) <[email protected]>
---
 fs/ntfs3/fslog.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)
fs/ntfs3/fslog.c +31 -2
index ed50c1d0c23e..1ddff43dbe87 100644
@@ -384,6 +384,14 @@ static inline u32 bytes_per_rt(const struct RESTART_TABLE *rt)
 	       sizeof(struct RESTART_TABLE);
 }
 
+static inline bool rstbl_entry_valid(const struct RESTART_TABLE *rt, u32 off)
+{
+	u16 size = le16_to_cpu(rt->size);
+
+	return size && off >= sizeof(*rt) && off < bytes_per_rt(rt) &&
+	       !((off - sizeof(*rt)) % size);
+}
+
 /* Log record length. */
 static inline u32 lrh_length(const struct LOG_REC_HDR *lr)
 {

@@ -5086,7 +5094,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
 		goto read_next_log_do_action;
 
 	t16 = le16_to_cpu(lrh->target_attr);
-	if (t16 >= bytes_per_rt(oatbl)) {
+	if (!rstbl_entry_valid(oatbl, t16)) {
 		err = -EINVAL;
 		goto out;
 	}

@@ -5231,8 +5239,29 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
 	if (lrh->undo_op == cpu_to_le16(Noop))
 		goto read_next_log_undo_action;
 
-	oe = Add2Ptr(oatbl, le16_to_cpu(lrh->target_attr));
+	/* Skip records with neither target nor LCN work. */
+	t16 = le16_to_cpu(lrh->undo_op);
+	if (!lrh->lcns_follow && !is_target_required(t16) &&
+	    can_skip_action(t16))
+		goto read_next_log_undo_action;
+
+	t16 = le16_to_cpu(lrh->target_attr);
+	if (!rstbl_entry_valid(oatbl, t16)) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	oe = Add2Ptr(oatbl, t16);
+	if (oe->next != RESTART_ENTRY_ALLOCATED_LE) {
+		err = -EINVAL;
+		goto out;
+	}
+
 	oa = oe->ptr;
+	if (!oa) {
+		err = -EINVAL;
+		goto out;
+	}
 
 	t16 = le16_to_cpu(lrh->lcns_follow);
 	if (!t16)

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935

Signature · 1 line
2.55.0
maintainer
#1 lore raw
Quoted 7 lines · Cen Zhang (Microsoft Security FORGE Labs)

log_replay() treats lrh->target_attr as a byte offset to an OPEN_ATTR_ENRTY in the open-attribute restart table. A valid offset is the table header size plus an integral number of entries. The redo lookup checks only target_attr < bytes_per_rt(oatbl); its earlier alignment check subtracts the header size without first checking the lower bound, so the subtraction can wrap.

Quoted 6 lines

The undo lookup has no table-bound check, and its earlier alignment check is skipped when lcns_follow is zero. Unlike the redo lookup, it also does not reject unallocated entries or NULL entry pointers.

A crafted $LogFile can therefore point into the table header, the middle of an entry, or past the table and trigger an out-of-bounds access.

Why not fix the userspace fsck tool instead?

thanks,

greg k-h