Re: [TOMOYO #9 (2.6.27-rc7-mm1) 1/6] LSM adapter functions.

Serge E. Hallyn
2008-09-25T17:00:26Z

Quoting Kentaro Takeda (takedakn@nttdata.co.jp):
> Signed-off-by: Kentaro Takeda <takedakn@nttdata.co.jp>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Toshiharu Harada <haradats@nttdata.co.jp>

So IMO there is some major badness here in the form of copying all of
those functions out of fs/namei.c. I think we need to discuss
case-by-case whether using the functions is appropriate (and hence
they should be made non-static in fs/namei.c), or whether the intended
goal should be met some other way. For instance:

> ---
> security/tomoyo/tomoyo.c | 341 +++++++++++++++++++++++++++++++++++++++++++++++
> security/tomoyo/tomoyo.h | 97 +++++++++++++
> 2 files changed, 438 insertions(+)
>

...

> +static int tmy_path_mknod(struct path *parent, struct dentry *dentry, int mode,
> + unsigned int dev)
> +{
> + struct vfsmount *mnt = parent->mnt;
> + struct inode *dir = parent->dentry->d_inode;
> + int error = 0;
> +
> + switch (mode & S_IFMT) {
> + case S_IFREG:
> + case 0:
> + error = may_create(dir, dentry);

Isn't may_create already done at the top of vfs_mknod() and vfs_create()?

> + if (error)
> + return error;
> + if (!dir->i_op || !dir->i_op->create)
> + return -EACCES; /* shouldn't it be ENOSYS? */
> + break;
> + case S_IFCHR:
> + case S_IFBLK:
> + case S_IFIFO:
> + case S_IFSOCK:
> + error = may_create(dir, dentry);

likewise...

> + if (error)
> + return error;
> + if ((S_ISCHR(mode) || S_ISBLK(mode)) && !capable(CAP_MKNOD))
> + return -EPERM;

likewise...

> + if (!dir->i_op || !dir->i_op->mknod)
> + return -EPERM;
> + error = devcgroup_inode_mknod(mode, dev);

likewise... (except in the case of fifo/sock, devcgroup should not be
consulted as I'm not sure it'll handle that properly - have you tested
tis with the device cgroup enabled?)

> + if (error)
> + return error;
> + break;
> + default:
> + return 0;
> + }
> + switch (mode & S_IFMT) {
> + case S_IFREG:
> + case 0:
> + error = tmy_check_1path_perm(TMY_TYPE_CREATE_ACL, dentry, mnt);
> + break;
> + case S_IFCHR:
> + error = tmy_check_1path_perm(TMY_TYPE_MKCHAR_ACL, dentry, mnt);
> + break;
> + case S_IFBLK:
> + error = tmy_check_1path_perm(TMY_TYPE_MKBLOCK_ACL, dentry, mnt);
> + break;
> + case S_IFIFO:
> + error = tmy_check_1path_perm(TMY_TYPE_MKFIFO_ACL, dentry, mnt);
> + break;
> + case S_IFSOCK:
> + error = tmy_check_1path_perm(TMY_TYPE_MKSOCK_ACL, dentry, mnt);
> + break;
> + }
> + return error;
> +}

-serge