lkml.org 
[lkml]   [2017]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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()...

\
 
 \ /
  Last update: 2017-12-01 18:40    [W:0.061 / U:1.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site