linux-kernel - Re: [PATCH] vfs: sanity check the length passed to inode_set_cached

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGudoHHeHKo6+R86pZTFSzAFRf2v=bc5LOGvbHmC0mCfkjRvgw@mail.gmail.com>
Date: Wed, 5 Feb 2025 17:20:43 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: brauner@...nel.org, viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] vfs: sanity check the length passed to inode_set_cached_link()

On Wed, Feb 5, 2025 at 5:12 PM Jan Kara <jack@...e.cz> wrote:
>
> On Tue 04-02-25 22:32:07, 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@...il.com>
>
> Yeah, it looks a bit pointless to pass the length in only to compare it
> against strlen(). But as a quick fix until we figure out something more
> clever it's fine I guess.
>

There is some dayjob stuff I need to take care of.

Once I'm both out from under the ruble *and* bored enough, I'll post
provisional support for VFS_WARN_ON and VFS_BUG_ON macros.

But that's for -next.

> Feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.cz>
>
>                                                                 Honza
>
> > ---
> >
> > 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
> >
> > 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
> >
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR



-- 
Mateusz Guzik <mjguzik gmail.com>