netdev - Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of 'xt_bpf_info

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7bbe72a8-dbbe-3343-765d-cc53eb40e0cd@iogearbox.net>
Date:   Fri, 1 Dec 2017 21:47:00 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Al Viro <viro@...IV.linux.org.uk>,
        Kees Cook <keescook@...omium.org>
Cc:     Shmulik Ladkani <shmulik.ladkani@...il.com>,
        Willem de Bruijn <willemb@...gle.com>,
        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 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()...