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

On Mon 17-08-26 17:24:56, Mateusz Guzik wrote:
> On Mon, Aug 17, 2026 at 10:17 AM Narek Jilavyan <[email protected]> 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 <[email protected]>
SUSE Labs, CR