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

Narek Jilavyan <[email protected]>
Newsgroups org.kernel.vger.linux-fsdevel,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
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 <[email protected]>
Signed-off-by: Narek Jilavyan <[email protected]>
---
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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.