Messages in this thread | | | Date | Sat, 28 Jan 2006 15:01:37 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH 2.6.16-rc1-git4] accessfs: a permission managing filesystem |
| |
Olaf Dietsche <olaf+list.linux-kernel@olafdietsche.de> wrote: > > Hi Andrew, > > Can you please include this patch in -mm, to give it wider testing?
I doubt if it'll get a lot of runtime testing.
> Accessfs is a permission managing filesystem. It allows to control > access to system resources, based on file permissions. It also > includes two modules. One module allows granting capabilities based > on user-/groupid. The second module allows to grant access to lower > numbered IP ports based on user-/groupid. >
It seems to be network-centric?
Do these capabilities really need to be implemented via a brand-new security infrastructure, rather then by enhancing the existing one(s)?
> + To use this option, you need to mount the access file system > + and do a chown on the appropriate ports: > + > + # mount -t accessfs none /proc/access > + # chown www /proc/access/net/ip/bind/80 > + # chown mail /proc/access/net/ip/bind/25
Documenting a feature in Kconfig is a bit odd. I assume proper Documentation is forthcoming?
> + */ > + > +#include <linux/accessfs_fs.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/security.h> > + > +static struct access_attr caps[29];
caps[ARRAY_SIZE(names)]
should work.
> +static const char *names[] = { > + "chown", > + "dac_override", > + "dac_read_search", > + "fowner", > + "fsetid", > + "kill", > + "setgid", > + "setuid", > + "setpcap", > + "linux_immutable", > + "net_bind_service", > + "net_broadcast", > + "net_admin", > + "net_raw", > + "ipc_lock", > + "ipc_owner", > + "sys_module", > + "sys_rawio", > + "sys_chroot", > + "sys_ptrace", > + "sys_pacct", > + "sys_admin", > + "sys_boot", > + "sys_nice", > + "sys_resource", > + "sys_time", > + "sys_tty_config", > + "mknod", > + "lease" > +}; > + > > +static void unregister_capabilities(struct accessfs_direntry *dir, int n) > +{ > + int i; > + for (i = 0; i < n; ++i) { > + accessfs_unregister(dir, names[i]); > + } > +}
Unneeded braces.
> +static int __init init_capabilities(void) > +{ > + struct accessfs_direntry *dir; > + int i, err; > + dir = accessfs_make_dirpath("capabilities"); > + if (dir == 0) > + return -ENOTDIR; > + > + for (i = 0; i < sizeof(caps) / sizeof(caps[0]); ++i) {
ARRAY_SIZE() (lots of instances)
> +static DECLARE_MUTEX(accessfs_sem);
Please use a `struct mutex'.
> + > +static inline void accessfs_readdir_aux(struct file *filp, struct accessfs_direntry *dir, int start, void *dirent, filldir_t filldir) > +{ > + struct list_head *list; > + int i; > + > + list = dir->children.next; > + for (i = 2; i < start && list != &dir->children; ++i) > + list = list->next; > + > + while (list != &dir->children) { > + struct accessfs_entry *de; > + de = list_entry(list, struct accessfs_entry, siblings); > + if (filldir(dirent, de->name, strlen(de->name), filp->f_pos, de->ino, DT_UNKNOWN) < 0) > + break; > + > + ++filp->f_pos; > + list = list->next; > + } > +}
Use standard list accessors?
Please fit code into 80 cols.
> +static int accessfs_readdir(struct file *filp, void *dirent, filldir_t filldir) > +{ > + int i; > + struct dentry *dentry = filp->f_dentry; > + struct accessfs_direntry *dir; > + > + i = filp->f_pos; > + switch (i) { > + case 0: > + if (filldir(dirent, ".", 1, i, dentry->d_inode->i_ino, DT_DIR) < 0) > + break; > + > + ++i; > + ++filp->f_pos; > + /* NO break; */ > + case 1: > + if (filldir(dirent, "..", 2, i, dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) > + break; > + > + ++i; > + ++filp->f_pos; > + /* NO break; */ > + default: > + down(&accessfs_sem); > + dir = (struct accessfs_direntry *) dentry->d_inode->u.generic_ip;
Unneeded typecast.
> + accessfs_readdir_aux(filp, dir, i, dirent, filldir); > + up(&accessfs_sem); > + break; > + } > + > + return 0; > +} > +
> +static void accessfs_init_inode(struct inode *inode, struct accessfs_entry *pe) > +{ > + static const struct timespec epoch = {0, 0};
Unneeded initialiser (although it does make things clearer)
> + > +static struct accessfs_direntry *accessfs_mkdir(struct accessfs_direntry *parent, const char *name, size_t len) > +{ > + int err; > + struct accessfs_direntry *dir; > + dir = kmalloc(sizeof(struct accessfs_direntry), GFP_KERNEL); > + if (dir == NULL) > + return NULL; > + > + dir->parent = parent; > + INIT_LIST_HEAD(&dir->children); > + err = accessfs_node_init(parent, &dir->node, name, len, &dir->attr, S_IFDIR | 0755); > + if (err) { > + kfree(dir); > + dir = 0; > + } > + > + return dir; > +}
Again, painful to read in 80-cols.
> +struct accessfs_direntry *accessfs_make_dirpath(const char *name) > +{ > + struct accessfs_direntry *dir = &accessfs_rootdir; > + const char *slash; > + down(&accessfs_sem);
Shouldn't that lock be per-superblock?
> +static void accessfs_read_inode(struct inode *inode) > +{ > + ino_t ino = inode->i_ino; > + struct list_head *list; > + down(&accessfs_sem); > + list_for_each(list, &hash) { > + struct accessfs_entry *pe; > + pe = list_entry(list, struct accessfs_entry, hash); > + if (pe->ino == ino) { > + accessfs_init_inode(inode, pe); > + break; > + } > + }
That's not a hash!
> +{ > + unregister_filesystem(&accessfs_fs_type); > + > +#ifdef CONFIG_PROC_FS > + remove_proc_entry("access",&proc_root); > +#endif > +}
The CONFIG_PROC_FS ifdefs shouldn't be needed - we have stubs.
> +static int accessfs_ip6_prot_sock(struct socket *sock, > + struct sockaddr *uaddr, int addr_len) > +{ > +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
That's a bit awkward, especially the CONFIG_IPV6_MODULE dependency. Is it possible to just unconditionally compile this in?
> +static int __init init_ip(void) > +{ > + struct accessfs_direntry *dir = accessfs_make_dirpath("net/ip/bind"); > + int i; > + bind_to_port = kmalloc(max_prot_sock * sizeof(*bind_to_port), GFP_KERNEL); > + if (bind_to_port == 0)
Use NULL to avoid sparse warnings.
> + > +#if CONFIG_ACCESSFS_PROT_SOCK < PROT_SOCK > +#define CONFIG_ACCESSFS_PROT_SOCK PROT_SOCK > +#elseif CONFIG_ACCESSFS_PROT_SOCK > 65536 > +#define CONFIG_ACCESSFS_PROT_SOCK 65536 > +#endif
Please don't redefine CONFIG_ variables like this. I'd have expected the compiler to have generated a warning about this, too.
> snum = ntohs(addr->sin_port); > err = -EACCES; > +#ifdef CONFIG_NET_HOOKS > + if (net_ops->ip_prot_sock(sock, uaddr, addr_len)) > +#else > if (snum && snum < PROT_SOCK && !capable(CAP_NET_BIND_SERVICE)) > +#endif > goto out;
Maybe some wrapper which hides the above?
> /* We keep a pair of addresses. rcv_saddr is the one > diff -urN a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c > --- a/net/ipv6/af_inet6.c Fri Jan 27 23:53:23 2006 > +++ b/net/ipv6/af_inet6.c Sat Jan 28 12:47:19 2006 > @@ -260,7 +260,11 @@ > return -EINVAL; > > snum = ntohs(addr->sin6_port); > +#ifdef CONFIG_NET_HOOKS > + if (net_ops->ip6_prot_sock(sock, uaddr, addr_len)) > +#else > if (snum && snum < PROT_SOCK && !capable(CAP_NET_BIND_SERVICE)) > +#endif > return -EACCES; >
which could be used here as well. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |