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

[Mateusz Guzik]

On Tue, Feb 4, 2025 at 10:32 PM Mateusz Guzik <mjguzik@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>
> This has a side effect of working around the panic reported in:
> https://lore.kernel.org/all/67a1e1f4.050a0220.163cdc.0063.GAE@xxxxxxxxxx/
>
> I'm confident this merely exposed a bug in ext4, see:
> https://lore.kernel.org/all/CAGudoHEv+Diti3r0x9VmF5ixgRVKk4trYnX_skVJNkQoTMaDHg@xxxxxxxxxxxxxx/#t
>
> 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);

I just remembered that path_noexec thing got a CVE.

I suspect someone may try to assign one here for user-mountable disk
images triggering this warn.

Can be patched to "printk_once", but then it does not implicitly tell
which fs is messing up due to lack of a backtrace.

> +               linklen = testlen;
> +       }
>         inode->i_link = link;
>         inode->i_linklen = linklen;
>         inode->i_opflags |= IOP_CACHED_LINK;
> --
> 2.43.0
>


-- 
Mateusz Guzik <mjguzik gmail.com>