Hi Andrew! Christoph! Al!
after thinking some time about the oracle words
(sent in reply to previous BME submissions) we
(Sam and I) came to the conclusion that it would
be a good idea to remove the nameidata introduced
in September 2003 from the inode permission()
checks, so that vfs_permission() can take care
of them ...
this is in two parts, the first one does the
removal and the second one fixes up nfs and fuse
by passing the relevant nd_flags via the mask
Note: this is just a suggestion, so please let
us know what you think
remove struct nameidata from *_permission() and
related security functions.
Signed-off-by: Herbert P?tzl <[email protected]>
Signed-off-by: Sam Vilain <[email protected]>
---
diff -NurpP --minimal linux-2.6.16-rc5/fs/cifs/cifsfs.c linux-2.6.16-rc5-vfs0.02.1/fs/cifs/cifsfs.c
--- linux-2.6.16-rc5/fs/cifs/cifsfs.c 2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/cifs/cifsfs.c 2006-02-28 05:08:53 +0100
@@ -220,7 +220,7 @@ cifs_statfs(struct super_block *sb, stru
longer available? */
}
-static int cifs_permission(struct inode * inode, int mask, struct nameidata *nd)
+static int cifs_permission(struct inode * inode, int mask)
{
struct cifs_sb_info *cifs_sb;
diff -NurpP --minimal linux-2.6.16-rc5/fs/coda/dir.c linux-2.6.16-rc5-vfs0.02.1/fs/coda/dir.c
--- linux-2.6.16-rc5/fs/coda/dir.c 2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/coda/dir.c 2006-02-28 05:08:18 +0100
@@ -151,7 +151,7 @@ exit:
}
-int coda_permission(struct inode *inode, int mask, struct nameidata *nd)
+int coda_permission(struct inode *inode, int mask)
{
int error = 0;
diff -NurpP --minimal linux-2.6.16-rc5/fs/coda/pioctl.c linux-2.6.16-rc5-vfs0.02.1/fs/coda/pioctl.c
--- linux-2.6.16-rc5/fs/coda/pioctl.c 2004-08-14 12:55:47 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/coda/pioctl.c 2006-02-28 06:04:48 +0100
@@ -24,8 +24,7 @@
#include <linux/coda_psdev.h>
/* pioctl ops */
-static int coda_ioctl_permission(struct inode *inode, int mask,
- struct nameidata *nd);
+static int coda_ioctl_permission(struct inode *inode, int mask);
static int coda_pioctl(struct inode * inode, struct file * filp,
unsigned int cmd, unsigned long user_data);
@@ -42,8 +41,7 @@ struct file_operations coda_ioctl_operat
};
/* the coda pioctl inode ops */
-static int coda_ioctl_permission(struct inode *inode, int mask,
- struct nameidata *nd)
+static int coda_ioctl_permission(struct inode *inode, int mask)
{
return 0;
}
diff -NurpP --minimal linux-2.6.16-rc5/fs/ext2/acl.c linux-2.6.16-rc5-vfs0.02.1/fs/ext2/acl.c
--- linux-2.6.16-rc5/fs/ext2/acl.c 2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/ext2/acl.c 2006-02-28 05:09:10 +0100
@@ -294,7 +294,7 @@ ext2_check_acl(struct inode *inode, int
}
int
-ext2_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext2_permission(struct inode *inode, int mask)
{
return generic_permission(inode, mask, ext2_check_acl);
}
diff -NurpP --minimal linux-2.6.16-rc5/fs/ext3/acl.c linux-2.6.16-rc5-vfs0.02.1/fs/ext3/acl.c
--- linux-2.6.16-rc5/fs/ext3/acl.c 2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/ext3/acl.c 2006-02-28 05:09:22 +0100
@@ -299,7 +299,7 @@ ext3_check_acl(struct inode *inode, int
}
int
-ext3_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext3_permission(struct inode *inode, int mask)
{
return generic_permission(inode, mask, ext3_check_acl);
}
diff -NurpP --minimal linux-2.6.16-rc5/fs/fuse/dir.c linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c
--- linux-2.6.16-rc5/fs/fuse/dir.c 2006-02-28 03:41:05 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c 2006-02-28 05:56:38 +0100
@@ -702,7 +702,7 @@ static int fuse_access(struct inode *ino
* access request is sent. Execute permission is still checked
* locally based on file mode.
*/
-static int fuse_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int fuse_permission(struct inode *inode, int mask)
{
struct fuse_conn *fc = get_fuse_conn(inode);
diff -NurpP --minimal linux-2.6.16-rc5/fs/hfs/inode.c linux-2.6.16-rc5-vfs0.02.1/fs/hfs/inode.c
--- linux-2.6.16-rc5/fs/hfs/inode.c 2006-02-28 03:41:05 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hfs/inode.c 2006-02-28 05:11:18 +0100
@@ -519,8 +519,7 @@ void hfs_clear_inode(struct inode *inode
}
}
-static int hfs_permission(struct inode *inode, int mask,
- struct nameidata *nd)
+static int hfs_permission(struct inode *inode, int mask)
{
if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
return 0;
diff -NurpP --minimal linux-2.6.16-rc5/fs/hfsplus/inode.c linux-2.6.16-rc5-vfs0.02.1/fs/hfsplus/inode.c
--- linux-2.6.16-rc5/fs/hfsplus/inode.c 2006-02-28 03:41:05 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hfsplus/inode.c 2006-02-28 05:11:30 +0100
@@ -237,7 +237,7 @@ static void hfsplus_set_perms(struct ino
perms->dev = cpu_to_be32(HFSPLUS_I(inode).dev);
}
-static int hfsplus_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int hfsplus_permission(struct inode *inode, int mask)
{
/* MAY_EXEC is also used for lookup, if no x bit is set allow lookup,
* open_exec has the same test, so it's still not executable, if a x bit
diff -NurpP --minimal linux-2.6.16-rc5/fs/hostfs/hostfs_kern.c linux-2.6.16-rc5-vfs0.02.1/fs/hostfs/hostfs_kern.c
--- linux-2.6.16-rc5/fs/hostfs/hostfs_kern.c 2006-01-03 17:29:56 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hostfs/hostfs_kern.c 2006-02-28 05:11:51 +0100
@@ -796,7 +796,7 @@ int hostfs_rename(struct inode *from_ino
return(err);
}
-int hostfs_permission(struct inode *ino, int desired, struct nameidata *nd)
+int hostfs_permission(struct inode *ino, int desired)
{
char *name;
int r = 0, w = 0, x = 0, err;
diff -NurpP --minimal linux-2.6.16-rc5/fs/hpfs/namei.c linux-2.6.16-rc5-vfs0.02.1/fs/hpfs/namei.c
--- linux-2.6.16-rc5/fs/hpfs/namei.c 2004-08-14 12:55:19 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hpfs/namei.c 2006-02-28 04:54:18 +0100
@@ -415,7 +415,7 @@ again:
d_drop(dentry);
spin_lock(&dentry->d_lock);
if (atomic_read(&dentry->d_count) > 1 ||
- permission(inode, MAY_WRITE, NULL) ||
+ permission(inode, MAY_WRITE) ||
!S_ISREG(inode->i_mode) ||
get_write_access(inode)) {
spin_unlock(&dentry->d_lock);
diff -NurpP --minimal linux-2.6.16-rc5/fs/jfs/acl.c linux-2.6.16-rc5-vfs0.02.1/fs/jfs/acl.c
--- linux-2.6.16-rc5/fs/jfs/acl.c 2005-10-28 20:49:44 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/jfs/acl.c 2006-02-28 05:12:01 +0100
@@ -140,7 +140,7 @@ static int jfs_check_acl(struct inode *i
return -EAGAIN;
}
-int jfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int jfs_permission(struct inode *inode, int mask)
{
return generic_permission(inode, mask, jfs_check_acl);
}
diff -NurpP --minimal linux-2.6.16-rc5/fs/jfs/jfs_acl.h linux-2.6.16-rc5-vfs0.02.1/fs/jfs/jfs_acl.h
--- linux-2.6.16-rc5/fs/jfs/jfs_acl.h 2005-10-28 20:49:44 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/jfs/jfs_acl.h 2006-02-28 06:07:00 +0100
@@ -20,7 +20,7 @@
#ifdef CONFIG_JFS_POSIX_ACL
-int jfs_permission(struct inode *, int, struct nameidata *);
+int jfs_permission(struct inode *, int);
int jfs_init_acl(tid_t, struct inode *, struct inode *);
int jfs_setattr(struct dentry *, struct iattr *);
diff -NurpP --minimal linux-2.6.16-rc5/fs/namei.c linux-2.6.16-rc5-vfs0.02.1/fs/namei.c
--- linux-2.6.16-rc5/fs/namei.c 2006-02-28 03:41:07 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/namei.c 2006-02-28 05:34:28 +0100
@@ -225,7 +225,7 @@ int generic_permission(struct inode *ino
return -EACCES;
}
-int permission(struct inode *inode, int mask, struct nameidata *nd)
+int permission(struct inode *inode, int mask)
{
int retval, submask;
@@ -250,13 +250,13 @@ int permission(struct inode *inode, int
/* Ordinary permission routines do not understand MAY_APPEND. */
submask = mask & ~MAY_APPEND;
if (inode->i_op && inode->i_op->permission)
- retval = inode->i_op->permission(inode, submask, nd);
+ retval = inode->i_op->permission(inode, submask);
else
retval = generic_permission(inode, submask, NULL);
if (retval)
return retval;
- return security_inode_permission(inode, mask, nd);
+ return security_inode_permission(inode, mask);
}
/**
@@ -271,7 +271,8 @@ int permission(struct inode *inode, int
*/
int vfs_permission(struct nameidata *nd, int mask)
{
- return permission(nd->dentry->d_inode, mask, nd);
+ /* do nd based stuff here */
+ return permission(nd->dentry->d_inode, mask);
}
/**
@@ -288,7 +289,8 @@ int vfs_permission(struct nameidata *nd,
*/
int file_permission(struct file *file, int mask)
{
- return permission(file->f_dentry->d_inode, mask, NULL);
+ /* do file based stuff here */
+ return permission(file->f_dentry->d_inode, mask);
}
/*
@@ -425,7 +427,7 @@ static int exec_permission_lite(struct i
return -EACCES;
ok:
- return security_inode_permission(inode, MAY_EXEC, nd);
+ return security_inode_permission(inode, MAY_EXEC);
}
/*
@@ -1219,7 +1221,8 @@ static struct dentry * __lookup_hash(str
int err;
inode = base->d_inode;
- err = permission(inode, MAY_EXEC, nd);
+ /* should become vfs_permission() */
+ err = permission(inode, MAY_EXEC);
dentry = ERR_PTR(err);
if (err)
goto out;
@@ -1354,7 +1357,7 @@ static int may_delete(struct inode *dir,
BUG_ON(victim->d_parent->d_inode != dir);
- error = permission(dir,MAY_WRITE | MAY_EXEC, NULL);
+ error = permission(dir,MAY_WRITE | MAY_EXEC);
if (error)
return error;
if (IS_APPEND(dir))
@@ -1391,7 +1394,8 @@ static inline int may_create(struct inod
return -EEXIST;
if (IS_DEADDIR(dir))
return -ENOENT;
- return permission(dir,MAY_WRITE | MAY_EXEC, nd);
+ /* should become vfs_permission() */
+ return permission(dir,MAY_WRITE | MAY_EXEC);
}
/*
@@ -2313,7 +2317,7 @@ static int vfs_rename_dir(struct inode *
* we'll need to flip '..'.
*/
if (new_dir != old_dir) {
- error = permission(old_dentry->d_inode, MAY_WRITE, NULL);
+ error = permission(old_dentry->d_inode, MAY_WRITE);
if (error)
return error;
}
diff -NurpP --minimal linux-2.6.16-rc5/fs/nfs/dir.c linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c
--- linux-2.6.16-rc5/fs/nfs/dir.c 2006-02-28 03:41:07 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c 2006-02-28 05:56:38 +0100
@@ -1635,7 +1635,7 @@ out:
return -EACCES;
}
-int nfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int nfs_permission(struct inode *inode, int mask)
{
struct rpc_cred *cred;
int res = 0;
diff -NurpP --minimal linux-2.6.16-rc5/fs/nfsd/nfsfh.c linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/nfsfh.c
--- linux-2.6.16-rc5/fs/nfsd/nfsfh.c 2005-06-22 02:38:37 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/nfsfh.c 2006-02-28 05:35:53 +0100
@@ -56,7 +56,7 @@ static int nfsd_acceptable(void *expv, s
/* make sure parents give x permission to user */
int err;
parent = dget_parent(tdentry);
- err = permission(parent->d_inode, MAY_EXEC, NULL);
+ err = permission(parent->d_inode, MAY_EXEC);
if (err < 0) {
dput(parent);
break;
diff -NurpP --minimal linux-2.6.16-rc5/fs/nfsd/vfs.c linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/vfs.c
--- linux-2.6.16-rc5/fs/nfsd/vfs.c 2006-02-28 03:41:07 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/vfs.c 2006-02-28 05:36:11 +0100
@@ -1817,12 +1817,12 @@ nfsd_permission(struct svc_export *exp,
inode->i_uid == current->fsuid)
return 0;
- err = permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC), NULL);
+ err = permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
/* Allow read access to binaries even when mode 111 */
if (err == -EACCES && S_ISREG(inode->i_mode) &&
acc == (MAY_READ | MAY_OWNER_OVERRIDE))
- err = permission(inode, MAY_EXEC, NULL);
+ err = permission(inode, MAY_EXEC);
return err? nfserrno(err) : 0;
}
diff -NurpP --minimal linux-2.6.16-rc5/fs/proc/base.c linux-2.6.16-rc5-vfs0.02.1/fs/proc/base.c
--- linux-2.6.16-rc5/fs/proc/base.c 2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/proc/base.c 2006-02-28 05:14:35 +0100
@@ -579,14 +579,14 @@ static int proc_check_root(struct inode
return proc_check_chroot(root, vfsmnt);
}
-static int proc_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int proc_permission(struct inode *inode, int mask)
{
if (generic_permission(inode, mask, NULL) != 0)
return -EACCES;
return proc_check_root(inode);
}
-static int proc_task_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int proc_task_permission(struct inode *inode, int mask)
{
struct dentry *root;
struct vfsmount *vfsmnt;
diff -NurpP --minimal linux-2.6.16-rc5/fs/reiserfs/xattr.c linux-2.6.16-rc5-vfs0.02.1/fs/reiserfs/xattr.c
--- linux-2.6.16-rc5/fs/reiserfs/xattr.c 2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/reiserfs/xattr.c 2006-02-28 05:14:55 +0100
@@ -1343,7 +1343,7 @@ static int reiserfs_check_acl(struct ino
return error;
}
-int reiserfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int reiserfs_permission(struct inode *inode, int mask)
{
/*
* We don't do permission checks on the internal objects.
diff -NurpP --minimal linux-2.6.16-rc5/fs/smbfs/file.c linux-2.6.16-rc5-vfs0.02.1/fs/smbfs/file.c
--- linux-2.6.16-rc5/fs/smbfs/file.c 2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/smbfs/file.c 2006-02-28 05:15:06 +0100
@@ -387,7 +387,7 @@ smb_file_release(struct inode *inode, st
* privileges, so we need our own check for this.
*/
static int
-smb_file_permission(struct inode *inode, int mask, struct nameidata *nd)
+smb_file_permission(struct inode *inode, int mask)
{
int mode = inode->i_mode;
int error = 0;
diff -NurpP --minimal linux-2.6.16-rc5/fs/xattr.c linux-2.6.16-rc5-vfs0.02.1/fs/xattr.c
--- linux-2.6.16-rc5/fs/xattr.c 2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/xattr.c 2006-02-28 05:35:18 +0100
@@ -58,7 +58,7 @@ xattr_permission(struct inode *inode, co
return -EPERM;
}
- return permission(inode, mask, NULL);
+ return permission(inode, mask);
}
int
diff -NurpP --minimal linux-2.6.16-rc5/fs/xfs/linux-2.6/xfs_iops.c linux-2.6.16-rc5-vfs0.02.1/fs/xfs/linux-2.6/xfs_iops.c
--- linux-2.6.16-rc5/fs/xfs/linux-2.6/xfs_iops.c 2006-02-28 03:41:09 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/xfs/linux-2.6/xfs_iops.c 2006-02-28 05:15:25 +0100
@@ -614,8 +614,7 @@ linvfs_put_link(
STATIC int
linvfs_permission(
struct inode *inode,
- int mode,
- struct nameidata *nd)
+ int mode)
{
vnode_t *vp = LINVFS_GET_VP(inode);
int error;
diff -NurpP --minimal linux-2.6.16-rc5/include/linux/fs.h linux-2.6.16-rc5-vfs0.02.1/include/linux/fs.h
--- linux-2.6.16-rc5/include/linux/fs.h 2006-02-28 03:41:18 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/include/linux/fs.h 2006-02-28 05:29:44 +0100
@@ -1040,7 +1040,7 @@ struct inode_operations {
void * (*follow_link) (struct dentry *, struct nameidata *);
void (*put_link) (struct dentry *, struct nameidata *, void *);
void (*truncate) (struct inode *);
- int (*permission) (struct inode *, int, struct nameidata *);
+ int (*permission) (struct inode *, int);
int (*setattr) (struct dentry *, struct iattr *);
int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *);
int (*setxattr) (struct dentry *, const char *,const void *,size_t,int);
@@ -1465,7 +1465,8 @@ extern int do_remount_sb(struct super_bl
void *data, int force);
extern sector_t bmap(struct inode *, sector_t);
extern int notify_change(struct dentry *, struct iattr *);
-extern int permission(struct inode *, int, struct nameidata *);
+extern int permission(struct inode *, int);
+// extern int vfs_permission(struct nameidata *, int);
extern int generic_permission(struct inode *, int,
int (*check_acl)(struct inode *, int));
diff -NurpP --minimal linux-2.6.16-rc5/include/linux/nfs_fs.h linux-2.6.16-rc5-vfs0.02.1/include/linux/nfs_fs.h
--- linux-2.6.16-rc5/include/linux/nfs_fs.h 2006-02-28 03:41:19 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/include/linux/nfs_fs.h 2006-02-28 05:58:49 +0100
@@ -296,7 +296,7 @@ extern struct inode *nfs_fhget(struct su
extern int nfs_refresh_inode(struct inode *, struct nfs_fattr *);
extern int nfs_post_op_update_inode(struct inode *inode, struct nfs_fattr *fattr);
extern int nfs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
-extern int nfs_permission(struct inode *, int, struct nameidata *);
+extern int nfs_permission(struct inode *, int);
extern int nfs_access_get_cached(struct inode *, struct rpc_cred *, struct nfs_access_entry *);
extern void nfs_access_add_cache(struct inode *, struct nfs_access_entry *);
extern int nfs_open(struct inode *, struct file *);
diff -NurpP --minimal linux-2.6.16-rc5/include/linux/security.h linux-2.6.16-rc5-vfs0.02.1/include/linux/security.h
--- linux-2.6.16-rc5/include/linux/security.h 2006-02-28 03:41:19 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/include/linux/security.h 2006-02-28 06:09:30 +0100
@@ -346,7 +346,6 @@ struct swap_info_struct;
* called when the actual read/write operations are performed.
* @inode contains the inode structure to check.
* @mask contains the permission mask.
- * @nd contains the nameidata (may be NULL).
* Return 0 if permission is granted.
* @inode_setattr:
* Check permission before setting file attributes. Note that the kernel
@@ -1157,7 +1156,7 @@ struct security_operations {
struct inode *new_dir, struct dentry *new_dentry);
int (*inode_readlink) (struct dentry *dentry);
int (*inode_follow_link) (struct dentry *dentry, struct nameidata *nd);
- int (*inode_permission) (struct inode *inode, int mask, struct nameidata *nd);
+ int (*inode_permission) (struct inode *inode, int mask);
int (*inode_setattr) (struct dentry *dentry, struct iattr *attr);
int (*inode_getattr) (struct vfsmount *mnt, struct dentry *dentry);
void (*inode_delete) (struct inode *inode);
@@ -1606,12 +1605,11 @@ static inline int security_inode_follow_
return security_ops->inode_follow_link (dentry, nd);
}
-static inline int security_inode_permission (struct inode *inode, int mask,
- struct nameidata *nd)
+static inline int security_inode_permission (struct inode *inode, int mask)
{
if (unlikely (IS_PRIVATE (inode)))
return 0;
- return security_ops->inode_permission (inode, mask, nd);
+ return security_ops->inode_permission (inode, mask);
}
static inline int security_inode_setattr (struct dentry *dentry,
@@ -2270,8 +2268,7 @@ static inline int security_inode_follow_
return 0;
}
-static inline int security_inode_permission (struct inode *inode, int mask,
- struct nameidata *nd)
+static inline int security_inode_permission (struct inode *inode, int mask)
{
return 0;
}
diff -NurpP --minimal linux-2.6.16-rc5/ipc/mqueue.c linux-2.6.16-rc5-vfs0.02.1/ipc/mqueue.c
--- linux-2.6.16-rc5/ipc/mqueue.c 2006-02-28 03:41:21 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/ipc/mqueue.c 2006-02-28 05:28:20 +0100
@@ -639,7 +639,7 @@ static int oflag2acc[O_ACCMODE] = { MAY_
return ERR_PTR(-EINVAL);
}
- if (permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE], NULL)) {
+ if (permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE])) {
dput(dentry);
mntput(mqueue_mnt);
return ERR_PTR(-EACCES);
diff -NurpP --minimal linux-2.6.16-rc5/security/dummy.c linux-2.6.16-rc5-vfs0.02.1/security/dummy.c
--- linux-2.6.16-rc5/security/dummy.c 2006-02-28 03:41:33 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/security/dummy.c 2006-02-28 05:16:36 +0100
@@ -324,7 +324,7 @@ static int dummy_inode_follow_link (stru
return 0;
}
-static int dummy_inode_permission (struct inode *inode, int mask, struct nameidata *nd)
+static int dummy_inode_permission (struct inode *inode, int mask)
{
return 0;
}
diff -NurpP --minimal linux-2.6.16-rc5/security/seclvl.c linux-2.6.16-rc5-vfs0.02.1/security/seclvl.c
--- linux-2.6.16-rc5/security/seclvl.c 2006-02-28 03:41:33 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/security/seclvl.c 2006-02-28 05:19:56 +0100
@@ -422,7 +422,7 @@ static void seclvl_bd_release(struct ino
* seclvl 1, we only deny writes to *mounted* block devices.
*/
static int
-seclvl_inode_permission(struct inode *inode, int mask, struct nameidata *nd)
+seclvl_inode_permission(struct inode *inode, int mask)
{
if (current->pid != 1 && S_ISBLK(inode->i_mode) && (mask & MAY_WRITE)) {
switch (seclvl) {
diff -NurpP --minimal linux-2.6.16-rc5/security/selinux/hooks.c linux-2.6.16-rc5-vfs0.02.1/security/selinux/hooks.c
--- linux-2.6.16-rc5/security/selinux/hooks.c 2006-02-28 03:41:34 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/security/selinux/hooks.c 2006-02-28 05:20:22 +0100
@@ -2052,12 +2052,11 @@ static int selinux_inode_follow_link(str
return dentry_has_perm(current, NULL, dentry, FILE__READ);
}
-static int selinux_inode_permission(struct inode *inode, int mask,
- struct nameidata *nd)
+static int selinux_inode_permission(struct inode *inode, int mask)
{
int rc;
- rc = secondary_ops->inode_permission(inode, mask, nd);
+ rc = secondary_ops->inode_permission(inode, mask);
if (rc)
return rc;
fixup nfs and fuse nameidata related checks by
passing the relevant flags via the mask.
Signed-off-by: Herbert P?tzl <[email protected]>
Acked-by: Sam Vilain <[email protected]>
---
diff -NurpP --minimal linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c linux-2.6.16-rc5-vfs0.02.2/fs/fuse/dir.c
--- linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c 2006-02-28 05:56:38 +0100
+++ linux-2.6.16-rc5-vfs0.02.2/fs/fuse/dir.c 2006-02-28 05:49:57 +0100
@@ -731,8 +731,9 @@ static int fuse_permission(struct inode
if ((mask & MAY_EXEC) && !S_ISDIR(mode) && !(mode & S_IXUGO))
return -EACCES;
- if (nd && (nd->flags & LOOKUP_ACCESS))
+ if (mask & LOOKUP_ACCESS)
return fuse_access(inode, mask);
+
return 0;
}
}
diff -NurpP --minimal linux-2.6.16-rc5-vfs0.02.1/fs/namei.c linux-2.6.16-rc5-vfs0.02.2/fs/namei.c
--- linux-2.6.16-rc5-vfs0.02.1/fs/namei.c 2006-02-28 05:34:28 +0100
+++ linux-2.6.16-rc5-vfs0.02.2/fs/namei.c 2006-02-28 05:47:29 +0100
@@ -271,8 +271,11 @@ int permission(struct inode *inode, int
*/
int vfs_permission(struct nameidata *nd, int mask)
{
+ int nd_flags = nd->flags &
+ (LOOKUP_ACCESS | LOOKUP_CREATE | LOOKUP_OPEN);
+
/* do nd based stuff here */
- return permission(nd->dentry->d_inode, mask);
+ return permission(nd->dentry->d_inode, mask | nd_flags);
}
/**
diff -NurpP --minimal linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c linux-2.6.16-rc5-vfs0.02.2/fs/nfs/dir.c
--- linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c 2006-02-28 05:56:38 +0100
+++ linux-2.6.16-rc5-vfs0.02.2/fs/nfs/dir.c 2006-02-28 05:51:36 +0100
@@ -1643,7 +1643,7 @@ int nfs_permission(struct inode *inode,
if (mask == 0)
goto out;
/* Is this sys_access() ? */
- if (nd != NULL && (nd->flags & LOOKUP_ACCESS))
+ if (mask & LOOKUP_ACCESS)
goto force_lookup;
switch (inode->i_mode & S_IFMT) {
@@ -1652,8 +1652,7 @@ int nfs_permission(struct inode *inode,
case S_IFREG:
/* NFSv4 has atomic_open... */
if (nfs_server_capable(inode, NFS_CAP_ATOMIC_OPEN)
- && nd != NULL
- && (nd->flags & LOOKUP_OPEN))
+ && (mask & LOOKUP_OPEN))
goto out;
break;
case S_IFDIR:
On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> Hi Andrew! Christoph! Al!
>
> after thinking some time about the oracle words
> (sent in reply to previous BME submissions) we
> (Sam and I) came to the conclusion that it would
> be a good idea to remove the nameidata introduced
> in September 2003 from the inode permission()
> checks, so that vfs_permission() can take care
> of them ...
Why? There may be perfectly legitimate reasons for the filesystem to
request information about the path. I can think of server failover
situations in NFSv4 where the client may need to look up the filehandle
for the file on the new server before it can service the ACCESS call.
> this is in two parts, the first one does the
> removal and the second one fixes up nfs and fuse
> by passing the relevant nd_flags via the mask
>
> Note: this is just a suggestion, so please let
> us know what you think
Firstly, the fact that the lookup intent flags happen not to collide
with MAY_* is a complete fluke, not a design. The numerical values of
either set of flags could change tomorrow for all you know.
Secondly, an intent is _not_ a permissions mask by any stretch of the
imagination.
IOW: at the very least make that intent flag a separate parameter.
Cheers,
Trond
> On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> Hi Andrew! Christoph! Al!
>
> after thinking some time about the oracle words
> (sent in reply to previous BME submissions) we
> (Sam and I) came to the conclusion that it would
> be a good idea to remove the nameidata introduced
> in September 2003 from the inode permission()
> checks, so that vfs_permission() can take care
> of them ...
Could you please provide a link to that 'previous BME submissions'?
Thanks.
Also, since you are modifying LSM interfaces, why not discuss it on the
LSM mailing list?
And finally, please don't remove nameidata. Modules out there depend on it
and we at Sophos are about to release a new product which needs it as
well. The plan was to announce the whole thing parallel with the release,
but after spotting your post I was prompted to react ahead of the
schedule. However, I am very busy at the moment so the actual announcment
with full details will have to wait for a week or two.
>
> And finally, please don't remove nameidata. Modules out there depend on it
are those modules about to merged into the kernel? The current intent
infrastructure isn't fulfilling what it should do well, and from what
I've seen on the discussions it sounds that the best way forward is to
undo the current implementation and then roll out one which caters to
the needs of the existing users better.
As external module, you have little say so far simply because your usage
isn't visible. I'd urge you to quickly submit your code so that the
things you need from this are better visible to the people who are
thinking and working on the redesign.
> and we at Sophos are about to release a new product which needs it as
> well.
I assume we're talking about an open source product, or at least kernel
component, here?
Arjan van de Ven <[email protected]> wrote on 01/03/2006 12:37:51:
> > And finally, please don't remove nameidata. Modules out there depend
on it
>
> are those modules about to merged into the kernel? The current intent
See third paragraph of my reply.
> infrastructure isn't fulfilling what it should do well, and from what
> I've seen on the discussions it sounds that the best way forward is to
> undo the current implementation and then roll out one which caters to
> the needs of the existing users better.
That I don't know so I can't comment at the moment. I haven't seen
anything on linux-security-modules recently?
> As external module, you have little say so far simply because your usage
> isn't visible. I'd urge you to quickly submit your code so that the
> things you need from this are better visible to the people who are
> thinking and working on the redesign.
I know all that, but it is a complicated matter to discuss. That's why I
was planning to make a comprehensive announcement which would discuss most
of the hot topics. Ideally yes, I would like to merge, but it won't happen
now. The first thing I would like to do is establish common ground with
other security vendors so that we could approach the problem together.
Personaly, I am not sure whether insisting that everything should be a
part of kernel is a right thing to do even though I think I understand all
the up- and down-sides of both policies.
Having said all this above, I am afraid that there will be no other choice
but to start working on the announcement asap. :)
> > and we at Sophos are about to release a new product which needs it as
> > well.
>
> I assume we're talking about an open source product, or at least kernel
> component, here?
Of course. Kernel component might be known to some as Talpa and it is
released under GPL.
On Wed, Mar 01, 2006 at 12:28:25PM +0000, [email protected] wrote:
> > On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> > Hi Andrew! Christoph! Al!
> >
> > after thinking some time about the oracle words
> > (sent in reply to previous BME submissions) we
> > (Sam and I) came to the conclusion that it would
> > be a good idea to remove the nameidata introduced
> > in September 2003 from the inode permission()
> > checks, so that vfs_permission() can take care
> > of them ...
>
> Could you please provide a link to that 'previous BME submissions'?
here you go: http://lkml.org/lkml/2006/1/21/19
> Thanks.
you're welcome!
> Also, since you are modifying LSM interfaces, why not discuss it on
> the LSM mailing list?
no problem with that, will cc the lsm folks next time
(feel free to bounce the messages)
for now, here is a link to this thread:
http://lkml.org/lkml/2006/2/28/4
> And finally, please don't remove nameidata. Modules out there depend
> on it and we at Sophos are about to release a new product which needs
> it as well. The plan was to announce the whole thing parallel with the
> release, but after spotting your post I was prompted to react ahead
> of the schedule. However, I am very busy at the moment so the actual
> announcment with full details will have to wait for a week or two.
thing is, permission() does inode based checks
and the nameidata is not even provided in most
cases, so you cannot rely on that information
anyway
it would probably be better to have some kind
of vfs_permission, which uses dentry/vfsmnt for
decisions on the vfs layer, this would also allow
to cover most of the cases where nameidata is not
available (for example the filep based stuff)
best,
Herbert
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Wed, Mar 01, 2006 at 12:45:44AM -0800, Trond Myklebust wrote:
> On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> > Hi Andrew! Christoph! Al!
> >
> > after thinking some time about the oracle words
> > (sent in reply to previous BME submissions) we
> > (Sam and I) came to the conclusion that it would
> > be a good idea to remove the nameidata introduced
> > in September 2003 from the inode permission()
> > checks, so that vfs_permission() can take care
> > of them ...
>
> Why? There may be perfectly legitimate reasons for the filesystem to
> request information about the path. I can think of server failover
> situations in NFSv4 where the client may need to look up the
> filehandle for the file on the new server before it can service the
> ACCESS call.
the second part is actually a hack to help nfs and fuse
to get the 'required' information until there is a proper
interface (at the vfs not inode level) to pass relevant
information (probably dentry/vfsmount/flags)
> > this is in two parts, the first one does the
> > removal and the second one fixes up nfs and fuse
> > by passing the relevant nd_flags via the mask
> >
> > Note: this is just a suggestion, so please let
> > us know what you think
>
> Firstly, the fact that the lookup intent flags happen not to collide
> with MAY_* is a complete fluke, not a design. The numerical values of
> either set of flags could change tomorrow for all you know.
>
> Secondly, an intent is _not_ a permissions mask by any stretch of the
> imagination.
see above
> IOW: at the very least make that intent flag a separate parameter.
IMHO it would be good to remove them completely form the
current permission() checks.
best,
Herbert
> Cheers,
> Trond
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
[email protected] wrote:
> Also, since you are modifying LSM interfaces, why not discuss it on the
> LSM mailing list?
>
> And finally, please don't remove nameidata. Modules out there depend on it
> and we at Sophos are about to release a new product which needs it as
> well. The plan was to announce the whole thing parallel with the release,
> but after spotting your post I was prompted to react ahead of the
> schedule. However, I am very busy at the moment so the actual announcment
> with full details will have to wait for a week or two.
You are treating the per-FS security hooks as if they were VFS security
hooks. This was an easy mistake to make, as the appearance of a (struct
nameidata*) sure makes it look like a VFS call.
However, most functions in the kernel don't pass anything in that
nameidata slot. Some (eg, syscalls that work on open FDs) can't,
either. So the fact that it does not guarantee VFS context information
in all situations means permission() is not a VFS function.
ie, we don't disagree with what you're trying to do, but if you want
path information then you should be working at the VFS layer, not the FS
layer.
Perhaps you could first come up with a patch to the LSM base that adds
VFS hooks rather than FS hooks and make your new system use those hooks?
I think that it might be more obvious where such hooks should go,
after applying this patch.
What we're aiming for, on the permission() front, is that all system
calls, ioctls, etc, call either vfs_permission() or file_permission().
Only those two functions should end up calling permission() directly.
Sam.
[email protected] wrote:
>>As external module, you have little say so far simply because your usage
>>isn't visible. I'd urge you to quickly submit your code so that the
>>things you need from this are better visible to the people who are
>>thinking and working on the redesign.
> I know all that, but it is a complicated matter to discuss. That's why I
> was planning to make a comprehensive announcement which would discuss most
> of the hot topics. Ideally yes, I would like to merge, but it won't happen
> now. The first thing I would like to do is establish common ground with
> other security vendors so that we could approach the problem together.
> Personaly, I am not sure whether insisting that everything should be a
> part of kernel is a right thing to do even though I think I understand all
> the up- and down-sides of both policies.
>
> Having said all this above, I am afraid that there will be no other choice
> but to start working on the announcement asap. :)
Perhaps you can put up the work in progress on a git repository somewhere?
Sam.
Trond Myklebust wrote:
>>after thinking some time about the oracle words
>>(sent in reply to previous BME submissions) we
>>(Sam and I) came to the conclusion that it would
>>be a good idea to remove the nameidata introduced
>>in September 2003 from the inode permission()
>>checks, so that vfs_permission() can take care
>>of them ...
> Why? There may be perfectly legitimate reasons for the filesystem to
> request information about the path.
Part of what we're trying to achieve is clearly seperating VFS from FS
operations. A lot of the problems with supporting the per-vfsmnt
options stemmed from this confusion.
In fact this was largely developed from your feedback on the prior
thread? :) we audited the uses, and you were right - some places really
couldn't set that field properly. After some playing and discussion, we
came to guess that this kind of cleanup was a good way to satisfy your
concerns as well as Christoph's?.
? http://xrl.us/j9et
? http://xrl.us/j9eo
The only place that the nameidata structure is currently used is in
those two places, and given that it *does* appear to represent FS vs VFS
brain damage to some extent, it's quite important we get this right
before umpteen security modules spring up using this bad hook ... the
API is broken (doesn't cover (struct file*)-based operations, only
lookup (struct nameidata*), and parameter is optional).
> I can think of server failover
> situations in NFSv4 where the client may need to look up the filehandle
> for the file on the new server before it can service the ACCESS call.
The current code doesn't seem to use that right now, at least not via
permission(). Either way, the information should be available - it just
needs to hook in somewhere else.
> Firstly, the fact that the lookup intent flags happen not to collide
> with MAY_* is a complete fluke, not a design. The numerical values of
> either set of flags could change tomorrow for all you know.
> Secondly, an intent is _not_ a permissions mask by any stretch of the
> imagination.
Yeah, I had this in there at one point as a safety check;
#if (_LOOKUP_MASK ^ _ACCESS_MASK) != (_LOOKUP_MASK | _ACCESS_MASK)
#error splode
#endif
You are right, it should either be converted to two parameters or the
combination made more formal.
> IOW: at the very least make that intent flag a separate parameter.
Yes, or rename it mask_and_intent perhaps if they were to be combined in
the same word (which makes sense, with a grand total of 7 bits being
used, and gcc not being in a position to spot that and optimise for us).
>
> Cheers,
> Trond
Thanks for your feedback!
Sam.
On Wed, 2006-03-01 at 14:11 +0100, Herbert Poetzl wrote:
> the second part is actually a hack to help nfs and fuse
> to get the 'required' information until there is a proper
> interface (at the vfs not inode level) to pass relevant
> information (probably dentry/vfsmount/flags)
The nameidata _IS_ the vfs structure for storing path context
information. You seem to be suggesting we need yet another one. Why?
> > > this is in two parts, the first one does the
> > > removal and the second one fixes up nfs and fuse
> > > by passing the relevant nd_flags via the mask
> > >
> > > Note: this is just a suggestion, so please let
> > > us know what you think
> >
> > Firstly, the fact that the lookup intent flags happen not to collide
> > with MAY_* is a complete fluke, not a design. The numerical values of
> > either set of flags could change tomorrow for all you know.
> >
> > Secondly, an intent is _not_ a permissions mask by any stretch of the
> > imagination.
>
> see above
>
> > IOW: at the very least make that intent flag a separate parameter.
>
> IMHO it would be good to remove them completely form the
> current permission() checks.
Vetoed!
Redundant RPC calls have performance costs to the client, the server and
the network. That intent information is there in order to allow the
filesystem to figure out whether or not it needs to do the permissions
check, or if that check is already being done by other operations.
Removing the intents are therefore not an option.
Cheers,
Trond
Trond Myklebust wrote:
>>the second part is actually a hack to help nfs and fuse
>>to get the 'required' information until there is a proper
>>interface (at the vfs not inode level) to pass relevant
>>information (probably dentry/vfsmount/flags)
> The nameidata _IS_ the vfs structure for storing path context
> information. You seem to be suggesting we need yet another one. Why?
Because you can't make a nameidata without a lookup, and file based
operations don't do a lookup. However you still have the vfsmnt and
inode hanging off the file struct.
Either that or we make a dummy nameidata structure for this situation,
possibly a filehandle relative lookup as used by openat() et al.
>>>Secondly, an intent is _not_ a permissions mask by any stretch of the
>>>imagination.
>>see above
>>>IOW: at the very least make that intent flag a separate parameter.
>>IMHO it would be good to remove them completely form the
>>current permission() checks.
> Vetoed!
> Redundant RPC calls have performance costs to the client, the server and
> the network. That intent information is there in order to allow the
> filesystem to figure out whether or not it needs to do the permissions
> check, or if that check is already being done by other operations.
> Removing the intents are therefore not an option.
OK, so we either make it an extra parameter or 'properly' stack them
into a single word. Do you have any preferences either way there?
Sam.
On Thu, 2006-03-02 at 14:35 +1300, Sam Vilain wrote:
> Trond Myklebust wrote:
> >>the second part is actually a hack to help nfs and fuse
> >>to get the 'required' information until there is a proper
> >>interface (at the vfs not inode level) to pass relevant
> >>information (probably dentry/vfsmount/flags)
> > The nameidata _IS_ the vfs structure for storing path context
> > information. You seem to be suggesting we need yet another one. Why?
>
> Because you can't make a nameidata without a lookup, and file based
> operations don't do a lookup. However you still have the vfsmnt and
> inode hanging off the file struct.
Which is fine by NFS, at least. We don't care about performing
permissions checks in those cases anyway, so a NULL nameidata is OK. The
only cases where we currently need to take action in ->permission() are
1) path walks (checking MAY_EXEC on the directory)
2) open() (on NFSv2/v3 only)
3) access()
In the future we can/should probably eliminate (2) by making an
atomic_open() for NFSv2/v3, so that we can correctly set the credential
that was used for the open() permissions check in the struct file.
> Either that or we make a dummy nameidata structure for this situation,
> possibly a filehandle relative lookup as used by openat() et al.
>
> >>>Secondly, an intent is _not_ a permissions mask by any stretch of the
> >>>imagination.
> >>see above
> >>>IOW: at the very least make that intent flag a separate parameter.
> >>IMHO it would be good to remove them completely form the
> >>current permission() checks.
> > Vetoed!
> > Redundant RPC calls have performance costs to the client, the server and
> > the network. That intent information is there in order to allow the
> > filesystem to figure out whether or not it needs to do the permissions
> > check, or if that check is already being done by other operations.
> > Removing the intents are therefore not an option.
>
> OK, so we either make it an extra parameter or 'properly' stack them
> into a single word. Do you have any preferences either way there?
An extra parameter should suffice for (1) and (2).
Cheers,
Trond