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

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

[Narek Jilavyan]

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, and notably the two whose
length comes from on-disk metadata (fs/ext4/inode.c, fs/erofs/inode.c)
both re-derive it and reject the inode rather than relying on this
helper.  The API should not require that of the next caller.

Validate unconditionally and fail safe: if the length disagrees, warn
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 instead of disclosing memory.

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

Fixes: ea3821990719 ("vfs: support caching symlink lengths in inodes")
Signed-off-by: Narek Jilavyan <njilav@gmail.com>
---
 include/linux/fs.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 50ce731a2..e1d8f2614 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -946,9 +946,19 @@ 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);
 	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.
+	 */
+	if (WARN_ON_ONCE(strlen(link) != linklen))
+		return;
+
 	inode->i_linklen = linklen;
 	inode->i_opflags |= IOP_CACHED_LINK;
 }
-- 
2.43.0

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

[Mateusz Guzik]

On Mon, Aug 17, 2026 at 10:17 AM Narek Jilavyan <njilav@gmail.com> wrote:
>
> 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, and notably the two whose
> length comes from on-disk metadata (fs/ext4/inode.c, fs/erofs/inode.c)
> both re-derive it and reject the inode rather than relying on this
> helper.  The API should not require that of the next caller.
>

I'm confused why you don't expect filesystems to guarantee correct
value here, but I can agree issuing the strlen is not a big deal
during inode setup and I'm not going to insist on NOT having it in
prod kernels.

However, if going that route, I think the proposed WARN_ON is too terse.

inode_set_cached_link() at some point had the following:
       testlen = strlen(link);
       if (testlen != linklen) {
               WARN_ONCE(1, "bad length passed for symlink [%s] (got
%d, expected %d)",
                         link, linklen, testlen);
               linklen = testlen;
       }

As in, it used the correct value to fix up the caller.

I think it is ok to refrain from caching.

The important bit is the extra information as to what's going on,
namely what's the len disparity. I guess the message would be nicer if
the it also printed the filesystem name. You can borrow code from
dump_inode to do it.


> Validate unconditionally and fail safe: if the length disagrees, warn
> 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 instead of disclosing memory.
>
> The check runs once per symlink inode setup, not once per readlink(),
> which is what the cache was for.
>
> Fixes: ea3821990719 ("vfs: support caching symlink lengths in inodes")
> Signed-off-by: Narek Jilavyan <njilav@gmail.com>
> ---
>  include/linux/fs.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 50ce731a2..e1d8f2614 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -946,9 +946,19 @@ 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);
>         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.
> +        */
> +       if (WARN_ON_ONCE(strlen(link) != linklen))
> +               return;
> +
>         inode->i_linklen = linklen;
>         inode->i_opflags |= IOP_CACHED_LINK;
>  }
> --
> 2.43.0
>

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

[Narek Jilavyan]

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

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

[Jan Kara]

On Mon 17-08-26 17:24:56, Mateusz Guzik wrote:
> On Mon, Aug 17, 2026 at 10:17 AM Narek Jilavyan <njilav@gmail.com> wrote:
> >
> > 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, and notably the two whose
> > length comes from on-disk metadata (fs/ext4/inode.c, fs/erofs/inode.c)
> > both re-derive it and reject the inode rather than relying on this
> > helper.  The API should not require that of the next caller.
> >
> 
> I'm confused why you don't expect filesystems to guarantee correct
> value here, but I can agree issuing the strlen is not a big deal
> during inode setup and I'm not going to insist on NOT having it in
> prod kernels.

I don't know but to me this looks like overly defensive programming... If
we call strlen() in inode_set_cached_link(), then why pass the length to it
as an argument in the first place?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Narek Jilavyan]

On Tue 18-08-26 23:35:53, Jan Kara wrote:
> I don't know but to me this looks like overly defensive programming... If
> we call strlen() in inode_set_cached_link(), then why pass the length to it
> as an argument in the first place?

You're right, and so was Mateusz.  Please drop this one.

Going back over the callers: erofs and ext4 both run strlen()/strnlen()
themselves and reject the inode as corrupted before they ever call the
helper, and shmem and ext4's create path pass a length derived from the
string they just wrote.  Every caller already guarantees the contract, and
the two that take the length from untrusted on-disk metadata verify it
independently of this helper.

I cited those same two callers in the commit message as evidence that the
API was fragile.  That was backwards - they are evidence that it works as
documented.  What actually remained was "a future caller might get it
wrong", which does not justify a strlen() on every symlink setup, and, as
you point out, leaves the length parameter with no purpose.

Sorry for the noise.

Thanks,
Narek