LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Jann Horn <jannh@google.com>
Cc: Richard Henderson <rth@twiddle.net>,
	Ivan Kokshaysky <ink@jurassic.park.msu.ru>,
	Matt Turner <mattst88@gmail.com>,
	linux-fsdevel@vger.kernel.org,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Theodore Ts'o <tytso@mit.edu>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	linux-alpha@vger.kernel.org, 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 17:51:12 +0100
Message-ID: <20180731165112.GJ30522@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20180731161025.189534-1-jannh@google.com>

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.

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...

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...

  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 [this message]
2018-07-31 19:50   ` Jann Horn

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=20180731165112.GJ30522@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=adilger.kernel@dilger.ca \
    --cc=david@fromorbit.com \
    --cc=ebiederm@xmission.com \
    --cc=ink@jurassic.park.msu.ru \
    --cc=jannh@google.com \
    --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 \
    /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