[lkml]   [2008]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH][RFC] Simple tamper-proof device filesystem.
    On Sun, 06 Jan 2008 15:20:00 +0900, Tetsuo Handa said:

    > --- linux-2.6-mm.orig/fs/ramfs/inode.c
    > +++ linux-2.6-mm/fs/ramfs/inode.c
    > @@ -36,6 +36,20 @@
    > #include <asm/uaccess.h>
    > #include "internal.h"
    > +static struct inode *__ramfs_get_inode(struct super_block *sb, int mode,
    > + dev_t dev, bool tmpfs_with_mac);
    > +
    > +#define TMPFS_WITH_MAC 1
    > +#define TMPFS_WITHOUT_MAC 0
    > +#include <linux/quotaops.h>
    > +
    > +#ifdef CONFIG_SYAORAN
    > +#include "syaoran.h"
    > +#include "syaoran_init.c"
    > +#include "syaoran_main.c"
    > +#include "syaoran_debug.c"
    > +#endif

    Ouch. The .c files should generally be built into their own .o files and
    then the Makefile should do something like

    obj-$(CONFIG_SYAORAN) += syaoran.o

    unless there's *really* good reasons for including .c files (such as an
    otherwise-messy variable-namespace issue or similar).

    Also, has this been double-checked to Do The Right Thing if you have
    *two* instances of ramfs mounted, one with Syaoran and one without? I don't
    know the code well enough to know if you found *all* the places you need
    something like:

    > - inode = ramfs_get_inode(dir->i_sb, S_IFLNK|S_IRWXUGO, 0);
    > +#ifdef CONFIG_SYAORAN
    > + /*** SYAORAN start. ***/
    > + if (dir->i_sb->s_op == &syaoran_ops) {
    > + if (syaoran_may_create_node(dentry, S_IFLNK, 0) < 0)
    > + return -EPERM;
    > + inode = syaoran_get_inode(dir->i_sb, S_IFLNK|S_IRWXUGO, 0);
    > + /*** SYAORAN end. ***/
    > + } else
    > +#endif
    > + inode = ramfs_get_inode(dir->i_sb, S_IFLNK|S_IRWXUGO, 0);

    (incidentally, all of these should probably be abstracted into a helper
    function that's 'static inline' so we have just one #ifdef in the definition
    in a .h file, and none in open .c code).

    Similarly for other places you have #ifdef CONFIG_ in ramfs .c code - see if
    you can abstract it out.

    > +/*
    > + * Original tmpfs doesn't set ramfs_dir_inode_operations.setattr field.
    > + * Now I'm setting the field to share tmpfs/rootfs/syaoran code.

    Question for the audience: *should* ramfs set that field so setattr works
    on ramfs (even if it's just a stub similar to the SELinux fscontext= mount

    Question for Tetsuo: What happens to this code if somebody actually does the
    above change?

    > --- linux-2.6-mm.orig/fs/Kconfig
    > +++ linux-2.6-mm/fs/Kconfig
    > @@ -978,6 +978,24 @@ config TMPFS_POSIX_ACL

    > + "Applications using well-known device locations under /dev
    > + get the device they want" (e.g. an application that accesses
    > + /dev/null can always get a character special device
    > + with major=1 and minor=3).

    This should say "will always get", not "can always", as this code will
    mandate, rather than just make possible.

    > + The list of possible combinations of filename and its attributes
    > + that can exist on this filesystem is defined at mount time
    > + using a configuration file.

    The format of this file needs to be documented. I'm not terribly thrilled by
    the idea of passing a file to be read by the kernel, but I also understand
    that if it isn't done before mount, you have a race condition betweet the
    mount and the load. Perhaps write some configfs code so that you can
    'mount /configfs; cat config.file > /configfs/syaoran; mount -t syaoran"?

    Similarly, it looks like you create your debug files inside the ramfs - that
    is probably a bad idea and possibly can exhaust resources. Convert it to
    use debugfs instead?

    > + if (!filename) {
    > + printk(KERN_INFO "SYAORAN: Missing config-file path.\n");
    > + return -EINVAL;

    Does this (and the code right after Do The Right Thing if somebody does this:

    mount -t syaoran -o noatime,noexec /some/path

    (I admit not knowing if mount options common to all mounts are stripped out
    by the VFS code or passed down to this code).

    Or even worse, "-o noatime,accept=/some/path/ramfs.cfg"?

    > + f = open_pathname(AT_FDCWD, filename, O_RDONLY, 0600);
    > + if (IS_ERR(f)) {
    > + printk(KERN_INFO "SYAORAN: Can't open '%s'\n", filename);
    > + return -EINVAL;
    > + }

    Does this do what you think it does if run in a chroot process or if
    some creative person does "accept=../../path/to/bad_data.cfg"?

    That printk should be KERN_ERR, I think.

    That's all that's immediately obvious to me - somebody who actually understands
    the filesystem code better will probably need to review it for all the stuff
    I missed before it can be included.
    [unhandled content-type:application/pgp-signature]
     \ /
      Last update: 2008-01-07 18:13    [W:0.037 / U:10.504 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site