[PATCH v2] fs: do not cache a symlink length that disagrees with the string

[Narek Jilavyan <njilav@gmail.com>]

inode_set_cached_link() stores a caller-supplied length in i_linklen and
sets IOP_CACHED_LINK.  vfs_readlink() then uses that length directly:

	if (inode->i_opflags & IOP_CACHED_LINK)
		return readlink_copy(buffer, buflen, inode->i_link,
				     inode->i_linklen);

and readlink_copy() clamps only against the user buffer, not against
the string, so a length larger than the symlink body becomes a
copy_to_user() of adjacent kernel memory - reachable by any process
calling readlink() on such a symlink.

The only thing standing behind the invariant is

	VFS_WARN_ON_INODE(strlen(link) != linklen, inode);

which expands to BUILD_BUG_ON_INVALID() unless CONFIG_DEBUG_VFS is set.
On a production kernel it type-checks the expression and evaluates
nothing, so the value is stored unvalidated.

All four in-tree callers are correct today.  This makes the helper
enforce its own contract rather than leaving it to the next caller.

Validate unconditionally and fail safe: if the length disagrees, warn
with the disparity and leave IOP_CACHED_LINK clear.  i_linklen has
exactly one reader in the tree and it is gated on that flag, and
vfs_readlink() falls back to i_link with a strlen() of its own, so the
inode degrades to the behaviour that predates the cached length rather
than disclosing memory.  That fallback state is not exotic: 19 other
filesystems set i_link directly and never set IOP_CACHED_LINK, so it is
exercised routinely.

The check runs once per symlink inode setup, not once per readlink(),
which is what the cache was for.

Tested on 7.2 with CONFIG_DEBUG_VFS=n by handing a 3-byte allocation
holding "AB" to inode_set_cached_link() with a declared length of 64:

  before: readlink() returns 64, copying 62 bytes of adjacent kernel
          heap to userspace
  after:  bad length passed for symlink [AB] (got 64, expected 2)
          readlink() returns 2

Fixes: ea3821990719 ("vfs: support caching symlink lengths in inodes")
Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: Narek Jilavyan <njilav@gmail.com>
---
v2:
 - warn with the actual length disparity instead of a bare WARN_ON_ONCE,
   as suggested by Mateusz Guzik.
 - the filesystem name is not included.  struct file_system_type is not
   complete where inode_set_cached_link() is defined (the helper is at
   include/linux/fs.h:947, the struct at :2280), and dump_inode() is
   declared and defined inside #ifdef CONFIG_DEBUG_VFS so it does not
   exist on the kernels this patch is about.  Both established by
   compiler error rather than assumption.  If the fs name is wanted I
   can move the warn out of line into fs/inode.c, though that needs an
   EXPORT_SYMBOL since ext4 and erofs can be built as modules.
 - still refrains from caching on a mismatch rather than fixing the
   length up, so a buggy caller is reported rather than silently
   corrected.

 include/linux/fs.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 50ce731a2b..c7051cfc6b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -946,9 +946,25 @@ static inline void inode_state_replace(struct inode *inode,
 
 static inline void inode_set_cached_link(struct inode *inode, char *link, int linklen)
 {
-	VFS_WARN_ON_INODE(strlen(link) != linklen, inode);
+	int testlen;
+
 	VFS_WARN_ON_INODE(inode->i_opflags & IOP_CACHED_LINK, inode);
 	inode->i_link = link;
+
+	/*
+	 * i_linklen is used as a copy_to_user() length by vfs_readlink(), so it
+	 * must not be taken on trust. If it disagrees with the string, leave
+	 * IOP_CACHED_LINK clear: vfs_readlink() then falls back to i_link and
+	 * recomputes the length with strlen(), which is what it did before the
+	 * cached length was introduced.
+	 */
+	testlen = strlen(link);
+	if (testlen != linklen) {
+		WARN_ONCE(1, "bad length passed for symlink [%s] (got %d, expected %d)",
+			  link, linklen, testlen);
+		return;
+	}
+
 	inode->i_linklen = linklen;
 	inode->i_opflags |= IOP_CACHED_LINK;
 }
-- 
2.43.0