[PATCH] fs/ntfs3: Fix heap overflow after ALIGN in mi_pack_runs()

[Alexandro Calo]

When run_pack() returns a non-aligned byte count, ALIGN(err, 8)
inflates it by 1-7 bytes, and since the destination of the memmove() is
computed from that inflated value, the entire attribute tail could be
copied 1-7 bytes past where the record buffer ends.

The memmove() destination is next + new_run_size - run_size. For it to
stay within the record, new_run_size must not exceed run_size + dsize,
the space actually available.

Since new_run_size =3D ALIGN(err, 8), a fix could be to ensures there is
enough room that whatever ALIGN does to err, the result still fits within
the available space. Restricting the budget of run_pack() should
guarantee that.

Something like:
    u32 avail =3D (run_size + dsize) & ~7u;
    err =3D run_pack(run, svcn, len, Add2Ptr(attr, run_off), avail, &plen);

Now, run_pack() should never return 0. As now, the function returns at
least 1. So this is fine. Negative values are caught as errors by
if(err<0).

The rounded-down avail may be smaller than the existing run size,
if avail=3D0 run_pack is called and will write run_buf[0] =3D 0; that will =
be
OOB, because the buffer size is zero, so imo the caller should avoid
calling run_pack() with avail=3D0.

After run_pack(), if plen =3D 0, then svcn + plen - 1 becomes svcn - 1.
So, after run_pack(), it may be worth ensuring if(!plen).

Lastly, I'm not sure if -ENOSPC is the right error code here, but
it seems reasonable for a space condition.

This heap out-of-bounds write requires a crafted filesystem image,
which is not in the kernel threat model, but fixing memory errors would be
nice to keep things secure.

Signed-off-by: Alexandro Calo <alexandro.calo@xxxxxxxxxxxxxxxxxx>
---
 fs/ntfs3/record.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c
index 32bdb034c2a3..2466dba15241 100644
--- a/fs/ntfs3/record.c
+++ b/fs/ntfs3/record.c
@@ -637,18 +637,32 @@ int mi_pack_runs(struct mft_inode *mi, struct ATTRIB =
*attr,
 	u32 run_size =3D asize - run_off;
 	u32 tail =3D used - aoff - asize;
 	u32 dsize =3D sbi->record_size - used;
+	u32 avail;
=20
 	/* Make a maximum gap in current record. */
 	memmove(next + dsize, next, tail);
=20
+	/* Leave room for the 8-byte ALIGN() to avoid OOB write */
+	avail =3D (run_size + dsize) & ~7u;
+
+	if (!avail) {
+		memmove(next, next + dsize, tail);
+		return -ENOSPC;
+	}
+
 	/* Pack as much as possible. */
-	err =3D run_pack(run, svcn, len, Add2Ptr(attr, run_off), run_size + dsize=
,
+	err =3D run_pack(run, svcn, len, Add2Ptr(attr, run_off), avail,
 		       &plen);
 	if (err < 0) {
 		memmove(next, next + dsize, tail);
 		return err;
 	}
=20
+	if (!plen) {
+		memmove(next, next + dsize, tail);
+		return -ENOSPC;
+	}
+
 	new_run_size =3D ALIGN(err, 8);
=20
 	memmove(next + new_run_size - run_size, next + dsize, tail);

base-commit: 0e35b9b6ec0ffcc5e23cbdec09f5c622ad532b53
--
2.47.3