Subject: Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of
 'xt_bpf_info_v1'
To: Al Viro <viro@ZenIV.linux.org.uk>, Kees Cook <keescook@chromium.org>
Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>,
        Willem de Bruijn <willemb@google.com>,
        Pablo Neira Ayuso <pablo@netfilter.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        David Miller <davem@davemloft.net>,
        LKML <linux-kernel@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        Christoph Hellwig <hch@infradead.org>,
        Thomas Garnier <thgarnie@google.com>, Jann Horn <jannh@google.com>
References: <CAGXu5j+ZCJ8rRK=DAKaeuNmGzVY7xo1o9uJyWPEijmg9gw4fKA@mail.gmail.com>
 <20171201013304.GM21978@ZenIV.linux.org.uk>
 <20171201034859.GN21978@ZenIV.linux.org.uk>
 <20171201045439.GO21978@ZenIV.linux.org.uk>
 <20171201173941.GP21978@ZenIV.linux.org.uk>
From: Daniel Borkmann <daniel@iogearbox.net>
Message-ID: <7bbe72a8-dbbe-3343-765d-cc53eb40e0cd@iogearbox.net>
Date: Fri, 1 Dec 2017 21:47:00 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <20171201173941.GP21978@ZenIV.linux.org.uk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3059
Lines: 89

On 12/01/2017 06:39 PM, Al Viro wrote:
[...]
> 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?

No, thanks for looking into this, and sorry for this fugly hack! :( Not
that this doesn't make it any better, but I think back then I took it
over from mqueue implementation ... should have known better and looking
into making this generic instead, sigh. The above looks good to me, so
no objections from my side and thanks for working on it!

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