Re: [PATCH] vfs: sanity check the length passed to inode_set_cached_link()

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Feb 04, 2025 at 10:32:07PM +0100, Mateusz Guzik wrote:
> This costs a strlen() call when instatianating a symlink.
> 
> Preferably it would be hidden behind VFS_WARN_ON (or compatible), but
> there is no such facility at the moment. With the facility in place the
> call can be patched out in production kernels.
> 
> In the meantime, since the cost is being paid unconditionally, use the
> result to a fixup the bad caller.
> 
> This is not expected to persist in the long run (tm).
> 
> Sample splat:
> bad length passed for symlink [/tmp/syz-imagegen43743633/file0/file0] (got 131109, expected 37)
> [rest of WARN blurp goes here]
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> This has a side effect of working around the panic reported in:
> https://lore.kernel.org/all/67a1e1f4.050a0220.163cdc.0063.GAE@google.com/
> 
> I'm confident this merely exposed a bug in ext4, see:
> https://lore.kernel.org/all/CAGudoHEv+Diti3r0x9VmF5ixgRVKk4trYnX_skVJNkQoTMaDHg@mail.gmail.com/#t

Yes, ext4 should not continue to load a fast symlink where i_size >
sizeof(i_data).  I'm a little surprised that nd_terminate_link exists to
paper over this situation, but the commit adding it (035146851cfa2fe) is
from 2008 when we were all young and naïve.

But yes, would be nice to have a CONFIG_VFS_DEBUG behind which to hide
assertions so that insufficient validation in ext4 will burble up
through Ted's test infrastructure. ;)

That said, I just saw this in ext4_get_link:

	nd_terminate_link(bh->b_data, inode->i_size,
			  inode->i_sb->s_blocksize - 1);

Which is writing into a buffer head without a transaction?  Granted
symlink targets in ext4 are written out to disk with the null terminator
so I guess that's not a big deal.

--D

> 
> Nonethelss, should help catch future problems.
> 
>  include/linux/fs.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index be3ad155ec9f..1437a3323731 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -791,6 +791,19 @@ struct inode {
>  
>  static inline void inode_set_cached_link(struct inode *inode, char *link, int linklen)
>  {
> +	int testlen;
> +
> +	/*
> +	 * TODO: patch it into a debug-only check if relevant macros show up.
> +	 * In the meantime, since we are suffering strlen even on production kernels
> +	 * to find the right length, do a fixup if the wrong value got passed.
> +	 */
> +	testlen = strlen(link);
> +	if (testlen != linklen) {
> +		WARN_ONCE(1, "bad length passed for symlink [%s] (got %d, expected %d)",
> +			  link, linklen, testlen);
> +		linklen = testlen;
> +	}
>  	inode->i_link = link;
>  	inode->i_linklen = linklen;
>  	inode->i_opflags |= IOP_CACHED_LINK;
> -- 
> 2.43.0
> 
>