Re: [PATCH][RFC] Simple tamper-proof device filesystem.

[Valdis . Kletnieks]

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
stuff)?

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.

Attachment: pgp00000.pgp
Description: PGP signature