Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751801AbdLARjz (ORCPT ); Fri, 1 Dec 2017 12:39:55 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:46164 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751077AbdLARjx (ORCPT ); Fri, 1 Dec 2017 12:39:53 -0500 Date: Fri, 1 Dec 2017 17:39:41 +0000 From: Al Viro To: Kees Cook Cc: Shmulik Ladkani , Willem de Bruijn , Daniel Borkmann , Pablo Neira Ayuso , Linus Torvalds , David Miller , LKML , Network Development , Christoph Hellwig , Thomas Garnier , Jann Horn Subject: Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of 'xt_bpf_info_v1' Message-ID: <20171201173941.GP21978@ZenIV.linux.org.uk> References: <20171201013304.GM21978@ZenIV.linux.org.uk> <20171201034859.GN21978@ZenIV.linux.org.uk> <20171201045439.GO21978@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171201045439.GO21978@ZenIV.linux.org.uk> User-Agent: Mutt/1.9.0 (2017-09-02) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5090 Lines: 129 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()...