Re: [patch 2/3] lsm: add bsdjail module

[Christoph Hellwig <hch@infradead.org>]

Your filesystem handling code is completely superflous (and buggy).  Please
remove all the code dealing with chroot-lookalikes.  In your userland script
you simpl have to clone(.., CLONE_NEWNS) to detach your namespace from your
parent, then you can lazly unmount all filesystems and setup your new namespace
before starting the jail.  The added advantage is that you don't need any
cludges to keep the user from exiting the chroot.

> +#include <linux/ip.h>
> +#include <net/ipv6.h>
> +#include <linux/mount.h>
> +#include <asm/uaccess.h>

Please always include <asm/*.h> headers after <linux/*.h>

> +#include <linux/smp_lock.h>

I don't see you using the BKL anywhere.

> 
>
>
> 
> +#include <linux/kref.h>

Why that many blank lines?

> +static int jail_debug = 0;

no need to initialize to 0

> +MODULE_PARM(jail_debug, "i");

please user module_param

> +static int secondary = 0;

again no need to itnialize.

> +	char *ip4_addr_name;  /* char * containing ip4 addr to use for jail */
> +	char *ip6_addr_name;  /* char * containing ip6 addr to use for jail */

How do you habdle non-ip networking?  This really needs to be handled
more generally.

> +	/* won't let ourselves be removed until this jail goes away */
> +	try_module_get(THIS_MODULE);

must be __module_get

> +/*
> + * LSM /proc/<pid>/attr hooks.
> + * You may write into /proc/<pid>/attr/exec:
> + *    root /some/path
> + *    ip 2.2.2.2
> + * These values will be used on the next exec() to set up your jail
> + *  (assuming you're not already in a jail)

That's a really awkward interface.

> +jail_file_send_sigiotask(struct task_struct *tsk, struct fown_struct *fown,
> +       int fd, int reason)
> +{
> +	struct file *file;
> +	struct jail_struct *tsec, *fsec;
> +
> +	if (!in_jail(current))
> +		return 0;
> +
> +        file = (struct file *) ((long)fown - offsetof(struct file, f_owner));

bah.  Please use container_of or better get lsm folks to just pass you
a struct file *

> +jail_proc_inode_permission(struct inode *inode, int mask,
> +				    struct nameidata *nd)
> +{
> +	struct jail_struct *tsec = current->security;
> +	struct dentry *dentry = nd->dentry;
> +	unsigned pid;
> +
> +	pid = name_to_int(dentry);
> +	if (pid == ~0U) {
> +		struct qstr *dname = &dentry->d_name;
> +		if (strcmp(dname->name, "scsi") == 0 ||
> +			strcmp(dname->name, "sys") == 0 ||
> +			strcmp(dname->name, "ide") == 0)
> +			return -EPERM;
> +		return 0;

oh, please.  Don't submit such a crap.

if you want to disable sysctl access do it on the sysctl, not procfs level.
And disabling access to /proc/ide and /proc/scsi as two very special cases
(what about /proc/md, /proc/cciss or /proc/cpqarray?) is totally bullocks,
if they allow hardware interaction without checking for capabailities
fix them in the driver code.

This half-aided security by obscurity crap _is_ going to bite later on.