[PATCH v2] freevxfs: fix off-page reads in the directory walkers

vxfs_readdir() and vxfs_find_entry() walk a directory page with an
intra-page offset, pg_ofs, and dereference kaddr + pg_ofs without bounding
it.  Nothing validates the page first: vxfs_check_page() was never
implemented, only a commented out call survives in vxfs_get_page().  Three
on-disk values steer the walk unchecked.

VXFS_DIRBLKOV(), 2 * d_nhash + 4, is added at a dirblock boundary and the
entry is dereferenced right after; the enclosing while (pg_ofs < PAGE_SIZE
&& pos < limit) only gates the next iteration.  d_nhash can move the
dereference 32 pages from the start of the page.

d_reclen is added at the end of each iteration.  There the loop condition
does run, but it only proves that pg_ofs is inside the page, not that an
entry header is: pg_ofs of PAGE_SIZE - 2 passes it, and d_reclen at offset
4 of the next entry is off the page.

d_namelen is never compared to its record.  vxfs_readdir() passes it to
dir_emit() as the length of d_name, which starts at offset 10 of the entry.
filldir64() rejects PATH_MAX and above, but only after verify_dirent_name()
has run memchr() over the name, and shorter names are copied out - up to
4095 bytes past the page.  vxfs_find_entry() memcmp()s up to 256.

A crafted image reaches all three, vxfs_readdir() from getdents64(2) and
vxfs_find_entry() from a path walk.  KASAN reports a 2-byte read past the
page in vxfs_readdir(), a read in memchr() below filldir64(), and a read in
memcmp() below vxfs_lookup().

Bound them: leave the page if the overhead moved pg_ofs out of it, require
the entry header and then the whole record to fit in the page, and skip an
entry whose name does not fit in its record.  A record shorter than its own
header is refused too, so a free entry cannot slide the next one by less
than an entry.  Where the page is abandoned, advance pos to its end so the
outer loop keeps making progress.

vxfs_readdir() also ended the walk on a page that emitted nothing, its
per-page rc conflating a full buffer with an empty page, and the new checks
add another way to reach that; track the full buffer directly.

Valid images are unaffected: a record holds its own name and does not cross
its dirblock, dirblocks do not cross a block, and sb_set_blocksize() has
already refused a block size larger than a page.

Found by XBOW.

Fixes: 12495ea3ac47 ("freevxfs: refactor readdir and lookup code")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Federico Kirschbaum <[email protected]>
Cc: [email protected]
Signed-off-by: Baul Lee <[email protected]>
---
v2, in reply to the Sashiko review Christian pointed at.  Its three points
are right.

v1 checked pg_ofs only after the dirblock overhead, which does not cover
the dereference: pg_ofs of PAGE_SIZE - 2 passes that check too, and
d_reclen alone reaches it.  A kernel carrying v1 still reports both reads.
The d_namelen one is the worse of the two and predates the 2016 refactor,
hence the second Fixes tag.  The premature end of the walk is real but is
not memory safety; it is in here because the new checks add a way to reach
it, and I can split it out if you prefer.

Not taken: 4 byte alignment for d_reclen.  It cannot be established from
d_reclen alone - VXFS_DIRBLKOV() is 2 * d_nhash + 4, so an odd d_nhash
already starts a dirblock's entries two bytes off, and such an image mounts
and lists fine before and after this patch.  A short record is refused,
which is the half that needs no guess.  The rest belongs with an
implementation of vxfs_check_page().

Verified on 7.2.0-rc5 with KASAN, on crafted images built from the driver's
own headers: before the patch getdents64() and stat() produce the three
reports above, after it none, and well-formed images list unchanged.

Review: https://sashiko.dev/#/patchset/20260726062514.43756-1-baul.lee%40xbow.com
v1: https://lore.kernel.org/linux-fsdevel/[email protected]/

 fs/freevxfs/vxfs_lookup.c | 69 +++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/fs/freevxfs/vxfs_lookup.c b/fs/freevxfs/vxfs_lookup.c
index 138e08de976e..50c428c63126 100644
--- a/fs/freevxfs/vxfs_lookup.c
+++ b/fs/freevxfs/vxfs_lookup.c
@@ -78,6 +78,7 @@ vxfs_find_entry(struct inode *ip, struct dentry *dp, struct page **ppp)
 
 		while (pg_ofs < PAGE_SIZE && pos < limit) {
 			struct vxfs_direct *de;
+			int reclen, nlen;
 
 			if ((pos & (bsize - 1)) < 4) {
 				struct vxfs_dirblk *dbp =
@@ -87,6 +88,14 @@ vxfs_find_entry(struct inode *ip, struct dentry *dp, struct page **ppp)
 
 				pos += overhead;
 				pg_ofs += overhead;
+				if (pg_ofs >= PAGE_SIZE)
+					break;
+			}
+
+			/* the entry header must fit in the page */
+			if (pg_ofs + VXFS_NAMEMIN > PAGE_SIZE) {
+				pos += PAGE_SIZE - pg_ofs;
+				break;
 			}
 			de = (struct vxfs_direct *)(kaddr + pg_ofs);
 
@@ -96,12 +105,25 @@ vxfs_find_entry(struct inode *ip, struct dentry *dp, struct page **ppp)
 				break;
 			}
 
-			pg_ofs += fs16_to_cpu(sbi, de->d_reclen);
-			pos += fs16_to_cpu(sbi, de->d_reclen);
+			/* and so must the whole record, header included */
+			reclen = fs16_to_cpu(sbi, de->d_reclen);
+			if (reclen < VXFS_NAMEMIN ||
+			    pg_ofs + reclen > PAGE_SIZE) {
+				pos += PAGE_SIZE - pg_ofs;
+				break;
+			}
+
+			pg_ofs += reclen;
+			pos += reclen;
 			if (!de->d_ino)
 				continue;
 
-			if (namelen != fs16_to_cpu(sbi, de->d_namelen))
+			/* the name must fit in the record */
+			nlen = fs16_to_cpu(sbi, de->d_namelen);
+			if (VXFS_NAMEMIN + nlen > reclen)
+				continue;
+
+			if (namelen != nlen)
 				continue;
 			if (!memcmp(name, de->d_name, namelen)) {
 				*ppp = pp;
@@ -218,7 +240,7 @@ vxfs_readdir(struct file *fp, struct dir_context *ctx)
 		struct page *pp;
 		char *kaddr;
 		int pg_ofs = pos & ~PAGE_MASK;
-		int rc = 0;
+		bool full = false;
 
 		pp = vxfs_get_page(ip->i_mapping, pos >> PAGE_SHIFT);
 		if (IS_ERR(pp))
@@ -228,6 +250,7 @@ vxfs_readdir(struct file *fp, struct dir_context *ctx)
 
 		while (pg_ofs < PAGE_SIZE && pos < limit) {
 			struct vxfs_direct *de;
+			int reclen, nlen;
 
 			if ((pos & (bsize - 1)) < 4) {
 				struct vxfs_dirblk *dbp =
@@ -237,6 +260,14 @@ vxfs_readdir(struct file *fp, struct dir_context *ctx)
 
 				pos += overhead;
 				pg_ofs += overhead;
+				if (pg_ofs >= PAGE_SIZE)
+					break;
+			}
+
+			/* the entry header must fit in the page */
+			if (pg_ofs + VXFS_NAMEMIN > PAGE_SIZE) {
+				pos += PAGE_SIZE - pg_ofs;
+				break;
 			}
 			de = (struct vxfs_direct *)(kaddr + pg_ofs);
 
@@ -246,23 +277,35 @@ vxfs_readdir(struct file *fp, struct dir_context *ctx)
 				break;
 			}
 
-			pg_ofs += fs16_to_cpu(sbi, de->d_reclen);
-			pos += fs16_to_cpu(sbi, de->d_reclen);
+			/* and so must the whole record, header included */
+			reclen = fs16_to_cpu(sbi, de->d_reclen);
+			if (reclen < VXFS_NAMEMIN ||
+			    pg_ofs + reclen > PAGE_SIZE) {
+				pos += PAGE_SIZE - pg_ofs;
+				break;
+			}
+
+			pg_ofs += reclen;
+			pos += reclen;
 			if (!de->d_ino)
 				continue;
 
-			rc = dir_emit(ctx, de->d_name,
-					fs16_to_cpu(sbi, de->d_namelen),
-					fs32_to_cpu(sbi, de->d_ino),
-					DT_UNKNOWN);
-			if (!rc) {
+			/* the name must fit in the record */
+			nlen = fs16_to_cpu(sbi, de->d_namelen);
+			if (VXFS_NAMEMIN + nlen > reclen)
+				continue;
+
+			if (!dir_emit(ctx, de->d_name, nlen,
+				      fs32_to_cpu(sbi, de->d_ino),
+				      DT_UNKNOWN)) {
 				/* the dir entry was not read, fix pos. */
-				pos -= fs16_to_cpu(sbi, de->d_reclen);
+				pos -= reclen;
+				full = true;
 				break;
 			}
 		}
 		vxfs_put_page(pp);
-		if (!rc)
+		if (full)
 			break;
 	}
 
-- 
2.53.0