[PATCH v2] fs: do not cache a symlink length that disagrees with the string
Narek Jilavyan <[email protected]>
| Newsgroups | gmane.linux.file-systems,gmane.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