LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Jann Horn <jannh@google.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: rth@twiddle.net, ink@jurassic.park.msu.ru,
	Matt Turner <mattst88@gmail.com>,
	linux-fsdevel@vger.kernel.org,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	"Theodore Y. Ts'o" <tytso@mit.edu>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	linux-alpha@vger.kernel.org,
	kernel list <linux-kernel@vger.kernel.org>,
	Dave Chinner <david@fromorbit.com>, Pavel Machek <pavel@ucw.cz>
Subject: Re: [PATCH v2] fs: don't let getdents return bogus names
Date: Tue, 31 Jul 2018 21:50:08 +0200
Message-ID: <CAG48ez2WUWCdicfUk5ytP8sv6yfwDeW-stCrnfwkYhqBe8d_pw@mail.gmail.com> (raw)
In-Reply-To: <20180731165112.GJ30522@ZenIV.linux.org.uk>

On Tue, Jul 31, 2018 at 6:51 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Jul 31, 2018 at 06:10:27PM +0200, Jann Horn wrote:
> > +/*
> > + * Most filesystems don't filter out bogus directory entry names, and userspace
> > + * can get very confused by such names. Behave as if a low-level IO error had
> > + * happened while reading directory entries.
> > + */
> > +bool bogus_dirent_name(int *errp, const char *name, int namlen,
> > +                    const char *caller)
> > +{
> > +     if (namlen == 0) {
> > +             pr_err_once("%s: filesystem returned bogus empty name\n",
> > +                         caller);
> > +             *errp = -EUCLEAN;
> > +             return true;
> > +     }
> > +     if (memchr(name, '/', namlen)) {
> > +             pr_err_once("%s: filesystem returned bogus name '%*pEhp' (contains slash)\n",
> > +                         caller, namlen, name);
> > +             *errp = -EUCLEAN;
> > +             return true;
> > +     }
> > +     return false;
> > +}
>
> > +     if (bogus_dirent_name(&buf->result, name, namlen, __func__))
> > +             return -EUCLEAN;
>
> These calling conventions st^Ware rather suboptimal.  First of all,
>         * none of ->actor() callbacks will ever get called directly.
>         * there are only 4 callers.  3 of them (all in fs.h) are
> of the form return ....->actor(...) == 0;  The fourth is
>         return orig_ctx->actor(orig_ctx, name, namelen, offset, ino, d_type);
> in ovl_fill_real(), which itself is an ->actor() callback.
>
> So all these "return -E..." in the instances are completely pointless;
> we should just turn filldir_t into pointer-to-function-returning-bool
> and get rid of that boilerplate, rather than adding more to it.

Do you want me to try to write a patch that does that change?

> Furthermore, who the hell cares which callback has stepped into it?
> "The first time it happened from getdents(2) in a 32bit process and
> that's all you'll ever get out of me" seems to be less than helpful...

Yeah, true. Should I just remove the method name from the pr_err_once?
Or should I also use one of the _ratelimited things and print multiple
messages?

> And frankly, I would prefer
>         buf->result = check_dirent_name(name, namelen);
>         if (unlikely(buf->result))
>                 return false;
> making that thing return -EUCLEAN or 0.  Quite possibly - inlining it
> as well...

I guess that would conform to normal kernel coding standards a bit better.

Thanks for the quick feedback!

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-31 16:10 Jann Horn
2018-07-31 16:51 ` Al Viro
2018-07-31 19:50   ` Jann Horn [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAG48ez2WUWCdicfUk5ytP8sv6yfwDeW-stCrnfwkYhqBe8d_pw@mail.gmail.com \
    --to=jannh@google.com \
    --cc=adilger.kernel@dilger.ca \
    --cc=david@fromorbit.com \
    --cc=ebiederm@xmission.com \
    --cc=ink@jurassic.park.msu.ru \
    --cc=linux-alpha@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mattst88@gmail.com \
    --cc=pavel@ucw.cz \
    --cc=rth@twiddle.net \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git