Message-ID: <20171201173941.GP21978@ZenIV.linux.org.uk>
Date:   Fri, 1 Dec 2017 17:39:41 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Kees Cook <keescook@...omium.org>
Cc:     Shmulik Ladkani <shmulik.ladkani@...il.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        David Miller <davem@...emloft.net>,
        LKML <linux-kernel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Christoph Hellwig <hch@...radead.org>,
        Thomas Garnier <thgarnie@...gle.com>,
        Jann Horn <jannh@...gle.com>
Subject: Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of
 'xt_bpf_info_v1'

On Fri, Dec 01, 2017 at 04:54:39AM +0000, Al Viro wrote:
> On Fri, Dec 01, 2017 at 03:48:59AM +0000, Al Viro wrote:
> 
> > Something similar to get_prog_path_type() above might make for a usable
> > primitive, IMO...
> 
> Incidentally, bpf_obj_get_user()/bpf_obj_do_get() should just use
> user_path(), rather than wanking with getname()+kern_path(pname->name)+putname().
> Note that kern_path() will do getname_kernel() to get struct pathname...
> 
> Would cause problems for tracepoints in there, though.  And that, BTW,
> is precisely why I don't want tracepoints in core VFS, TYVM - makes
> restructuring the code harder...

Egads...  Contortions in bpf ->mknod() are really obnoxious.

First of all, it checks that ->d_fsdata is non-NULL and fails otherwise.
The only time ->d_fsdata gets non-NULL on that fs?  In bpf_obj_do_pin(), this:
        dentry->d_fsdata = raw;
        ret = vfs_mknod(dir, dentry, mode, devt);
        dentry->d_fsdata = NULL;
In other words, it's *not* going to work from normal mknod(2).  Why go through
->mknod(), then, especially since it requires that kind of contortions to
pass the data in?

devt is 0:1 or 0:2 here.  mode?  Character or block device, right?  Like hell -
it's a regular file.  And devt is a cute way to pass a flag down into bpf_mkobj()
(aka. ->mknod()) through vfs_mknod().  No, it doesn't go into ->i_rdev...
And to make the things even more fun, the damn thing is passed to a couple
of Linux S&M hooks - security_path_mknod() and security_inode_mknod().  Oh, sorry -
three hooks.  There's devcgroup_inode_mknod() as well, but that thing sees S_IFREG
in mode and buggers off quietly.  Our esteemed sadomaso^Wsecurity community gets
to play, though.  Without any way to see _what_ are we attaching to that place in
the bpf fs tree, but hey - it's security, it doesn't need to make sense...

What the hell?  If you need a clean way to do something, why don't you describe
(on fsdevel, or in off-list mail to relevant people) what do you really want?
Sure, you can "work around" anything, but doesn't that level of perversion
strike you as a clear sign of something being not right?

For crying out loud, you are trying to pass a tagged pointer to one or another
kind of object into your own function.  For that you
	* use a field in a globally visible data structure as a temporary storage
for a pointer
	* encode your tag (essentially a boolean) into a fucking _device_ _number_,
of all things, and shove it through, hoping that no LSM module gets weirded out by
non-zero device number combined with regular file for mode.

If that does not scream "wrong or missing primitive", I don't know what would.
You want something along the lines of "create a filesystem object at given
location, calling this function with this argument for actual object creation"?
Fair enough, but then let's add a primitive that would do just that.

And grepping around for similar sick tricks catches a slightly milder example -
mq_open(2) doesn't play with encoding stuff into dev_t, but otherwise it's very
similar and could also benefit from the same primitive.

How about something like this:
int vfs_mkobj(struct dentry *dentry, umode_t mode,
                int (*f)(struct dentry *, umode_t, void *),
		void *arg)
{
	struct inode *dir = dentry->d_parent->d_inode;
        int error = may_create(dir, dentry);
        if (error)
                return error;

        mode &= S_IALLUGO;
        mode |= S_IFREG;
        error = security_inode_create(dir, dentry, mode);
        if (error)
                return error;
        error = f(dentry, mode, arg);
        if (!error)
                fsnotify_create(dir, dentry);
        return error;
}

exported by fs/namei.c, with your code doing

	switch (type) {
	case BPF_TYPE_PROG:
		error = vfs_mkobj(path.dentry, mode, bpf_mkprog, raw);
		break;
	case BPF_TYPE_MAP:
		error = vfs_mkobj(path.dentry, mode, bpf_mkmap, raw);
		break;
	default:
		error = -EPERM;
	}
instead that vfs_mknod() hack, with

static int bpf_mkprog(struct inode *dir, struct dentry *dentry,
		 umode_t mode, void *raw)
{
	return bpf_mkobj_ops(dir, dentry, mode, raw, &bpf_prog_iops);
}

static int bpf_mkmap(struct inode *dir, struct dentry *dentry,
		 umode_t mode, void *raw)
{
	return bpf_mkobj_ops(dir, dentry, mode, raw, &bpf_map_iops);
}

static int bpf_mkobj_ops(struct inode *dir, struct dentry *dentry,
		 umode_t mode, void *raw, struct inode_operations *iops)
{
        struct inode *inode;

        inode = bpf_get_inode(dir->i_sb, dir, mode);
        if (IS_ERR(inode))
                return PTR_ERR(inode);

        inode->i_op = iops;
        inode->i_private = raw;

        bpf_dentry_finalize(dentry, inode, dir);
        return 0;
}

And to hell with messing with dev_t, ->d_fsdata or having ->mknod() there at all...
Might want to replace security_path_mknod() with something saner, while we are
at it.

Objections?

PS: mqueue.c would also benefit from such primitive - do_create() there would
simply pass attr as callback's argument into vfs_mkobj(), with callback being
the guts of mqueue_create()...

Powered by blists - more mailing lists