lkml.org 
[lkml]   [2006]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2006-01-29 00:04    [W:0.047 / U:59.900 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site