[RFC] Changes to the LSM file-related hooks for 2.5.58

[RFC] Changes to the LSM file-related hooks for 2.5.58

[Stephen D. Smalley]

This patch contains several changes to the LSM file-related hooks for
2.5.58, split out from the lsm-2.5 BitKeeper tree.  This patch can be
further split into a separate patch per change if desired.  Logically,
the changes are:

1) Add a security_sb_kern_mount hook call to the do_kern_mount
function.  This hook enables initialization of the superblock security
information of all superblock objects.  Placing a hook in
do_kern_mount was originally suggested by Al Viro.  This hook is used
by SELinux to setup the superblock security state and eliminated the
need for the superblock_precondition function.

2) Remove the security_inode_post_lookup hook entirely and add a
security_d_instantiate hook call to the d_instantiate function and the
d_splice_alias function.  The inode_post_lookup hook was subject to
races since the inode is already accessible through the dcache before
it is called, didn't handle filesystems that directly populate the
dcache, and wasn't always called in the desired context (e.g. for
pipe, shmem, and devpts inodes).  The d_instantiate hook enables
initialization of the inode security information.  This hook is used
by SELinux and by DTE to setup the inode security state, and
eliminated the need for the inode_precondition function in SELinux.

3) Add a security_file_alloc call to init_private_file and create a
release_private_file function to encapsulate release of private file
structures.  These changes ensure that security structures for private
files will be allocated and freed appropriately.

If anyone has any objections to these changes, please let me know.

 fs/dcache.c              |    3 +++
 fs/exportfs/expfs.c      |    3 +--
 fs/file_table.c          |   22 ++++++++++++++++++----
 fs/namei.c               |    9 +++------
 fs/nfsd/vfs.c            |    3 +--
 fs/super.c               |    8 ++++++++
 include/linux/fs.h       |    1 +
 include/linux/security.h |   37 +++++++++++++++++++++----------------
 kernel/ksyms.c           |    1 +
 security/dummy.c         |   19 +++++++++++++------
 10 files changed, 70 insertions(+), 36 deletions(-)
------

===== fs/dcache.c 1.37 vs edited =====
--- 1.37/fs/dcache.c	Mon Nov 18 07:37:59 2002
+++ edited/fs/dcache.c	Tue Jan 14 08:17:03 2003
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/mount.h>
 #include <asm/uaccess.h>
+#include <linux/security.h>
 
 #define DCACHE_PARANOIA 1
 /* #define DCACHE_DEBUG 1 */
@@ -699,6 +700,7 @@
 void d_instantiate(struct dentry *entry, struct inode * inode)
 {
 	if (!list_empty(&entry->d_alias)) BUG();
+	security_d_instantiate(entry, inode);
 	spin_lock(&dcache_lock);
 	if (inode)
 		list_add(&entry->d_alias, &inode->i_dentry);
@@ -825,6 +827,7 @@
 	struct dentry *new = NULL;
 
 	if (inode && S_ISDIR(inode->i_mode)) {
+		security_d_instantiate(dentry, inode);
 		spin_lock(&dcache_lock);
 		if (!list_empty(&inode->i_dentry)) {
 			new = list_entry(inode->i_dentry.next, struct dentry, d_alias);
===== fs/file_table.c 1.16 vs edited =====
--- 1.16/fs/file_table.c	Tue Nov 26 14:29:39 2002
+++ edited/fs/file_table.c	Tue Jan 14 08:17:03 2003
@@ -98,6 +98,7 @@
  */
 int init_private_file(struct file *filp, struct dentry *dentry, int mode)
 {
+	int error;
 	memset(filp, 0, sizeof(*filp));
 	eventpoll_init_file(filp);
 	filp->f_mode   = mode;
@@ -106,10 +107,23 @@
 	filp->f_uid    = current->fsuid;
 	filp->f_gid    = current->fsgid;
 	filp->f_op     = dentry->d_inode->i_fop;
-	if (filp->f_op->open)
-		return filp->f_op->open(dentry->d_inode, filp);
-	else
-		return 0;
+	error = security_file_alloc(filp);
+	if (!error)
+		if (filp->f_op->open) {
+			error = filp->f_op->open(dentry->d_inode, filp);
+			if (error)
+				security_file_free(filp);
+		}
+	return error;
+}
+
+void release_private_file(struct file *file)
+{
+	struct inode * inode = file->f_dentry->d_inode;
+
+	if (file->f_op && file->f_op->release)
+		file->f_op->release(inode, file);
+	security_file_free(file);
 }
 
 void fput(struct file * file)
===== fs/namei.c 1.63 vs edited =====
--- 1.63/fs/namei.c	Sat Jan 11 00:06:26 2003
+++ edited/fs/namei.c	Tue Jan 14 08:17:03 2003
@@ -372,10 +372,8 @@
 			result = dir->i_op->lookup(dir, dentry);
 			if (result)
 				dput(dentry);
-			else {
+			else
 				result = dentry;
-				security_inode_post_lookup(dir, result);
-			}
 		}
 		up(&dir->i_sem);
 		return result;
@@ -916,10 +914,9 @@
 		if (!new)
 			goto out;
 		dentry = inode->i_op->lookup(inode, new);
-		if (!dentry) {
+		if (!dentry)
 			dentry = new;
-			security_inode_post_lookup(inode, dentry);
-		} else
+		else
 			dput(new);
 	}
 out:
===== fs/super.c 1.93 vs edited =====
--- 1.93/fs/super.c	Tue Dec 31 21:44:36 2002
+++ edited/fs/super.c	Tue Jan 14 08:17:03 2003
@@ -610,6 +610,7 @@
 	struct file_system_type *type = get_fs_type(fstype);
 	struct super_block *sb = ERR_PTR(-ENOMEM);
 	struct vfsmount *mnt;
+	int error;
 
 	if (!type)
 		return ERR_PTR(-ENODEV);
@@ -620,6 +621,13 @@
 	sb = type->get_sb(type, flags, name, data);
 	if (IS_ERR(sb))
 		goto out_mnt;
+ 	error = security_sb_kern_mount(sb);
+ 	if (error) {
+ 		up_write(&sb->s_umount);
+ 		deactivate_super(sb);
+ 		sb = ERR_PTR(error);
+ 		goto out_mnt;
+ 	}
 	mnt->mnt_sb = sb;
 	mnt->mnt_root = dget(sb->s_root);
 	mnt->mnt_mountpoint = sb->s_root;
===== fs/exportfs/expfs.c 1.9 vs edited =====
--- 1.9/fs/exportfs/expfs.c	Thu Oct 10 19:07:34 2002
+++ edited/fs/exportfs/expfs.c	Tue Jan 14 08:17:03 2003
@@ -381,8 +381,7 @@
 	}
 
 out_close:
-	if (file.f_op->release)
-		file.f_op->release(dir, &file);
+	release_private_file(&file);
 out:
 	return error;
 }
===== fs/nfsd/vfs.c 1.55 vs edited =====
--- 1.55/fs/nfsd/vfs.c	Fri Jan 10 20:00:12 2003
+++ edited/fs/nfsd/vfs.c	Tue Jan 14 08:17:03 2003
@@ -491,8 +491,7 @@
 	struct dentry	*dentry = filp->f_dentry;
 	struct inode	*inode = dentry->d_inode;
 
-	if (filp->f_op->release)
-		filp->f_op->release(inode, filp);
+	release_private_file(filp);
 	if (filp->f_mode & FMODE_WRITE)
 		put_write_access(inode);
 }
===== include/linux/fs.h 1.210 vs edited =====
--- 1.210/include/linux/fs.h	Wed Jan  8 15:37:23 2003
+++ edited/include/linux/fs.h	Tue Jan 14 08:17:03 2003
@@ -493,6 +493,7 @@
 #define file_count(x)	atomic_read(&(x)->f_count)
 
 extern int init_private_file(struct file *, struct dentry *, int);
+extern void release_private_file(struct file *file);
 
 #define	MAX_NON_LFS	((1UL<<31) - 1)
 
===== include/linux/security.h 1.9 vs edited =====
--- 1.9/include/linux/security.h	Wed Dec 18 09:10:50 2002
+++ edited/include/linux/security.h	Tue Jan 14 08:17:03 2003
@@ -339,10 +339,6 @@
  *	@mnt is the vfsmount where the dentry was looked up
  *	@dentry contains the dentry structure for the file.
  *	Return 0 if permission is granted.
- * @inode_post_lookup:
- *	Set the security attributes for a file after it has been looked up.
- *	@inode contains the inode structure for parent directory.
- *	@d contains the dentry structure for the file.
  * @inode_delete:
  *	@inode contains the inode structure for deleted inode.
  *	This hook is called when a deleted inode is released (i.e. an inode
@@ -814,6 +810,7 @@
 
 	int (*sb_alloc_security) (struct super_block * sb);
 	void (*sb_free_security) (struct super_block * sb);
+	int (*sb_kern_mount) (struct super_block *sb);
 	int (*sb_statfs) (struct super_block * sb);
 	int (*sb_mount) (char *dev_name, struct nameidata * nd,
 			 char *type, unsigned long flags, void *data);
@@ -867,7 +864,6 @@
 	int (*inode_permission_lite) (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_post_lookup) (struct inode *inode, struct dentry *d);
         void (*inode_delete) (struct inode *inode);
 	int (*inode_setxattr) (struct dentry *dentry, char *name, void *value,
 			       size_t size, int flags);
@@ -952,6 +948,8 @@
 	                          struct security_operations *ops);
 	int (*unregister_security) (const char *name,
 	                            struct security_operations *ops);
+
+	void (*d_instantiate) (struct dentry * dentry, struct inode * inode);
 };
 
 /* global variables */
@@ -1034,6 +1032,11 @@
 	security_ops->sb_free_security (sb);
 }
 
+static inline int security_sb_kern_mount (struct super_block *sb)
+{
+	return security_ops->sb_kern_mount (sb);
+}
+
 static inline int security_sb_statfs (struct super_block *sb)
 {
 	return security_ops->sb_statfs (sb);
@@ -1240,12 +1243,6 @@
 	return security_ops->inode_getattr (mnt, dentry);
 }
 
-static inline void security_inode_post_lookup (struct inode *inode,
-					       struct dentry *dentry)
-{
-	security_ops->inode_post_lookup (inode, dentry);
-}
-
 static inline void security_inode_delete (struct inode *inode)
 {
 	security_ops->inode_delete (inode);
@@ -1543,6 +1540,11 @@
 	return security_ops->sem_semop(sma, sops, nsops, alter);
 }
 
+static inline void security_d_instantiate (struct dentry *dentry, struct inode *inode)
+{
+	security_ops->d_instantiate (dentry, inode);
+}
+
 /* prototypes */
 extern int security_scaffolding_startup	(void);
 extern int register_security	(struct security_operations *ops);
@@ -1550,7 +1552,6 @@
 extern int mod_reg_security	(const char *name, struct security_operations *ops);
 extern int mod_unreg_security	(const char *name, struct security_operations *ops);
 
-
 #else /* CONFIG_SECURITY */
 
 /*
@@ -1639,6 +1640,11 @@
 static inline void security_sb_free (struct super_block *sb)
 { }
 
+static inline int security_sb_kern_mount (struct super_block *sb)
+{
+	return 0;
+}
+
 static inline int security_sb_statfs (struct super_block *sb)
 {
 	return 0;
@@ -1817,10 +1823,6 @@
 	return 0;
 }
 
-static inline void security_inode_post_lookup (struct inode *inode,
-					       struct dentry *dentry)
-{ }
-
 static inline void security_inode_delete (struct inode *inode)
 { }
 
@@ -2103,6 +2105,9 @@
 {
 	return 0;
 }
+
+static inline void security_d_instantiate (struct dentry *dentry, struct inode *inode)
+{ }
 
 #endif	/* CONFIG_SECURITY */
 
===== kernel/ksyms.c 1.178 vs edited =====
--- 1.178/kernel/ksyms.c	Mon Jan 13 04:24:04 2003
+++ edited/kernel/ksyms.c	Tue Jan 14 08:17:03 2003
@@ -180,6 +180,7 @@
 EXPORT_SYMBOL(__mark_inode_dirty);
 EXPORT_SYMBOL(get_empty_filp);
 EXPORT_SYMBOL(init_private_file);
+EXPORT_SYMBOL(release_private_file);
 EXPORT_SYMBOL(filp_open);
 EXPORT_SYMBOL(filp_close);
 EXPORT_SYMBOL(put_filp);
===== security/dummy.c 1.14 vs edited =====
--- 1.14/security/dummy.c	Wed Dec 18 09:11:56 2002
+++ edited/security/dummy.c	Tue Jan 14 08:17:03 2003
@@ -120,6 +120,11 @@
 	return;
 }
 
+static int dummy_sb_kern_mount (struct super_block *sb)
+{
+	return 0;
+}
+
 static int dummy_sb_statfs (struct super_block *sb)
 {
 	return 0;
@@ -306,11 +311,6 @@
 	return 0;
 }
 
-static void dummy_inode_post_lookup (struct inode *ino, struct dentry *d)
-{
-	return;
-}
-
 static void dummy_inode_delete (struct inode *ino)
 {
 	return;
@@ -607,6 +607,12 @@
 	return -EINVAL;
 }
 
+static void dummy_d_instantiate (struct dentry *dentry, struct inode *inode)
+{
+	return;
+}
+
+
 struct security_operations dummy_security_ops;
 
 #define set_to_dummy_if_null(ops, function)				\
@@ -635,6 +641,7 @@
 	set_to_dummy_if_null(ops, bprm_check_security);
 	set_to_dummy_if_null(ops, sb_alloc_security);
 	set_to_dummy_if_null(ops, sb_free_security);
+	set_to_dummy_if_null(ops, sb_kern_mount);
 	set_to_dummy_if_null(ops, sb_statfs);
 	set_to_dummy_if_null(ops, sb_mount);
 	set_to_dummy_if_null(ops, sb_check_sb);
@@ -668,7 +675,6 @@
 	set_to_dummy_if_null(ops, inode_permission_lite);
 	set_to_dummy_if_null(ops, inode_setattr);
 	set_to_dummy_if_null(ops, inode_getattr);
-	set_to_dummy_if_null(ops, inode_post_lookup);
 	set_to_dummy_if_null(ops, inode_delete);
 	set_to_dummy_if_null(ops, inode_setxattr);
 	set_to_dummy_if_null(ops, inode_getxattr);
@@ -725,5 +731,6 @@
 	set_to_dummy_if_null(ops, sem_semop);
 	set_to_dummy_if_null(ops, register_security);
 	set_to_dummy_if_null(ops, unregister_security);
+	set_to_dummy_if_null(ops, d_instantiate);
 }
 


--
Stephen Smalley, NSA
sds@epoch.ncsc.mil

Re: [RFC] Changes to the LSM file-related hooks for 2.5.58

[Stephen D. Smalley]

"Christoph Hellwig" <hch@infradead.org> writes:
> Once you're at it please aqdd an argument to set file.f_flags so the
> caller can set O_LARGEFILE properly.  The (unmerged) XFS dmapi code wants
> that.
and
>Oh, and this comment was and still is wrong 8)

Thanks for your suggestion.  A revised patch for this logical change is
below.  The 'mode' argument is replaced with a 'flags' argument, and
the f_mode is derived from it, as in dentry_open().  The comment is
removed, but f_op is tested before dereferencing, also as in
dentry_open().

 fs/exportfs/expfs.c |    5 ++---
 fs/file_table.c     |   35 +++++++++++++++++++++++++++--------
 fs/nfsd/vfs.c       |    9 +++------
 include/linux/fs.h  |    5 ++++-
 kernel/ksyms.c      |    3 ++-
 5 files changed, 38 insertions(+), 19 deletions(-)
-----

===== fs/file_table.c 1.16 vs edited =====
--- 1.16/fs/file_table.c	Tue Nov 26 14:29:39 2002
+++ edited/fs/file_table.c	Wed Jan 15 13:43:51 2003
@@ -93,23 +93,42 @@
 
 /*
  * Clear and initialize a (private) struct file for the given dentry,
- * and call the open function (if any).  The caller must verify that
- * inode->i_fop is not NULL.
+ * allocate the security structure, and call the open function (if any).  
+ * The file should be released using close_private_file.
  */
-int init_private_file(struct file *filp, struct dentry *dentry, int mode)
+int open_private_file(struct file *filp, struct dentry *dentry, int flags)
 {
+	int error;
 	memset(filp, 0, sizeof(*filp));
 	eventpoll_init_file(filp);
-	filp->f_mode   = mode;
+	filp->f_flags  = flags;
+	filp->f_mode   = (flags+1) & O_ACCMODE;
 	atomic_set(&filp->f_count, 1);
 	filp->f_dentry = dentry;
 	filp->f_uid    = current->fsuid;
 	filp->f_gid    = current->fsgid;
 	filp->f_op     = dentry->d_inode->i_fop;
-	if (filp->f_op->open)
-		return filp->f_op->open(dentry->d_inode, filp);
-	else
-		return 0;
+	error = security_file_alloc(filp);
+	if (!error)
+		if (filp->f_op && filp->f_op->open) {
+			error = filp->f_op->open(dentry->d_inode, filp);
+			if (error)
+				security_file_free(filp);
+		}
+	return error;
+}
+
+/*
+ * Release a private file by calling the release function (if any) and
+ * freeing the security structure.
+ */
+void close_private_file(struct file *file)
+{
+	struct inode * inode = file->f_dentry->d_inode;
+
+	if (file->f_op && file->f_op->release)
+		file->f_op->release(inode, file);
+	security_file_free(file);
 }
 
 void fput(struct file * file)
===== fs/exportfs/expfs.c 1.9 vs edited =====
--- 1.9/fs/exportfs/expfs.c	Thu Oct 10 19:07:34 2002
+++ edited/fs/exportfs/expfs.c	Wed Jan 15 13:40:50 2003
@@ -353,7 +353,7 @@
 	/*
 	 * Open the directory ...
 	 */
-	error = init_private_file(&file, dentry, FMODE_READ);
+	error = open_private_file(&file, dentry, O_RDONLY);
 	if (error)
 		goto out;
 	error = -EINVAL;
@@ -381,8 +381,7 @@
 	}
 
 out_close:
-	if (file.f_op->release)
-		file.f_op->release(dir, &file);
+	close_private_file(&file);
 out:
 	return error;
 }
===== fs/nfsd/vfs.c 1.55 vs edited =====
--- 1.55/fs/nfsd/vfs.c	Fri Jan 10 20:00:12 2003
+++ edited/fs/nfsd/vfs.c	Wed Jan 15 13:42:02 2003
@@ -426,7 +426,7 @@
 {
 	struct dentry	*dentry;
 	struct inode	*inode;
-	int		flags = O_RDONLY|O_LARGEFILE, mode = FMODE_READ, err;
+	int		flags = O_RDONLY|O_LARGEFILE, err;
 
 	/*
 	 * If we get here, then the client has already done an "open",
@@ -463,14 +463,12 @@
 			goto out_nfserr;
 
 		flags = O_WRONLY|O_LARGEFILE;
-		mode  = FMODE_WRITE;
 
 		DQUOT_INIT(inode);
 	}
 
-	err = init_private_file(filp, dentry, mode);
+	err = open_private_file(filp, dentry, flags);
 	if (!err) {
-		filp->f_flags = flags;
 		filp->f_vfsmnt = fhp->fh_export->ex_mnt;
 	} else if (access & MAY_WRITE)
 		put_write_access(inode);
@@ -491,8 +489,7 @@
 	struct dentry	*dentry = filp->f_dentry;
 	struct inode	*inode = dentry->d_inode;
 
-	if (filp->f_op->release)
-		filp->f_op->release(inode, filp);
+	close_private_file(filp);
 	if (filp->f_mode & FMODE_WRITE)
 		put_write_access(inode);
 }
===== include/linux/fs.h 1.210 vs edited =====
--- 1.210/include/linux/fs.h	Wed Jan  8 15:37:23 2003
+++ edited/include/linux/fs.h	Wed Jan 15 13:39:31 2003
@@ -492,7 +492,10 @@
 #define get_file(x)	atomic_inc(&(x)->f_count)
 #define file_count(x)	atomic_read(&(x)->f_count)
 
-extern int init_private_file(struct file *, struct dentry *, int);
+/* Initialize and open a private file and allocate its security structure. */
+extern int open_private_file(struct file *, struct dentry *, int);
+/* Release a private file and free its security structure. */
+extern void close_private_file(struct file *file);
 
 #define	MAX_NON_LFS	((1UL<<31) - 1)
 
===== kernel/ksyms.c 1.178 vs edited =====
--- 1.178/kernel/ksyms.c	Mon Jan 13 04:24:04 2003
+++ edited/kernel/ksyms.c	Wed Jan 15 11:57:37 2003
@@ -179,7 +179,8 @@
 EXPORT_SYMBOL(end_buffer_io_sync);
 EXPORT_SYMBOL(__mark_inode_dirty);
 EXPORT_SYMBOL(get_empty_filp);
-EXPORT_SYMBOL(init_private_file);
+EXPORT_SYMBOL(open_private_file);
+EXPORT_SYMBOL(close_private_file);
 EXPORT_SYMBOL(filp_open);
 EXPORT_SYMBOL(filp_close);
 EXPORT_SYMBOL(put_filp);


 

--
Stephen Smalley, NSA
sds@epoch.ncsc.mil

Re: [RFC] Changes to the LSM file-related hooks for 2.5.58

[Christoph Hellwig]

On Wed, Jan 15, 2003 at 10:44:52AM -0500, Stephen D. Smalley wrote:
> + * caller must verify that inode->i_fop is not NULL.  The file should

Oh, and this comment was and still is wrong 8)

Re: [RFC] Changes to the LSM file-related hooks for 2.5.58

[Christoph Hellwig]

On Wed, Jan 15, 2003 at 10:44:52AM -0500, Stephen D. Smalley wrote:
> 
> "Andi Kleen" <ak@muc.de> writes:
> > Adding release_private_file requires fixing all code that uses 
> > init_private_file (including possible third party code). Otherwise
> > you have some subtle leak. It would better to rename init_private_file to
> > some other name and add appropiate comments so that this can be catched 
> > easily at compile time.
> 
> Thanks for the suggestion.  I've split out this logical change into a
> separate patch and reworked it in accordance with your suggestion.  See
> below.  Let me know if this does not address your concern.

Once you're at it please aqdd an argument to set file.f_flags so the
caller can set O_LARGEFILE properly.  The (unmerged) XFS dmapi code wants
that.

Re: [RFC] Changes to the LSM file-related hooks for 2.5.58

[Stephen D. Smalley]

"Andi Kleen" <ak@muc.de> writes:
> Adding release_private_file requires fixing all code that uses 
> init_private_file (including possible third party code). Otherwise
> you have some subtle leak. It would better to rename init_private_file to
> some other name and add appropiate comments so that this can be catched 
> easily at compile time.

Thanks for the suggestion.  I've split out this logical change into a
separate patch and reworked it in accordance with your suggestion.  See
below.  Let me know if this does not address your concern.

 fs/exportfs/expfs.c |    5 ++---
 fs/file_table.c     |   33 ++++++++++++++++++++++++++-------
 fs/nfsd/vfs.c       |    5 ++---
 include/linux/fs.h  |    5 ++++-
 kernel/ksyms.c      |    3 ++-
 5 files changed, 36 insertions(+), 15 deletions(-)
-----

===== fs/file_table.c 1.16 vs edited =====
--- 1.16/fs/file_table.c	Tue Nov 26 14:29:39 2002
+++ edited/fs/file_table.c	Wed Jan 15 09:39:17 2003
@@ -93,11 +93,13 @@
 
 /*
  * Clear and initialize a (private) struct file for the given dentry,
- * and call the open function (if any).  The caller must verify that
- * inode->i_fop is not NULL.
+ * allocate the security structure, and call the open function (if any).  The 
+ * caller must verify that inode->i_fop is not NULL.  The file should
+ * be released using close_private_file.
  */
-int init_private_file(struct file *filp, struct dentry *dentry, int mode)
+int open_private_file(struct file *filp, struct dentry *dentry, int mode)
 {
+	int error;
 	memset(filp, 0, sizeof(*filp));
 	eventpoll_init_file(filp);
 	filp->f_mode   = mode;
@@ -106,10 +108,27 @@
 	filp->f_uid    = current->fsuid;
 	filp->f_gid    = current->fsgid;
 	filp->f_op     = dentry->d_inode->i_fop;
-	if (filp->f_op->open)
-		return filp->f_op->open(dentry->d_inode, filp);
-	else
-		return 0;
+	error = security_file_alloc(filp);
+	if (!error)
+		if (filp->f_op->open) {
+			error = filp->f_op->open(dentry->d_inode, filp);
+			if (error)
+				security_file_free(filp);
+		}
+	return error;
+}
+
+/*
+ * Release a private file by calling the release function (if any) and
+ * freeing the security structure.
+ */
+void close_private_file(struct file *file)
+{
+	struct inode * inode = file->f_dentry->d_inode;
+
+	if (file->f_op && file->f_op->release)
+		file->f_op->release(inode, file);
+	security_file_free(file);
 }
 
 void fput(struct file * file)
===== fs/exportfs/expfs.c 1.9 vs edited =====
--- 1.9/fs/exportfs/expfs.c	Thu Oct 10 19:07:34 2002
+++ edited/fs/exportfs/expfs.c	Wed Jan 15 09:38:53 2003
@@ -353,7 +353,7 @@
 	/*
 	 * Open the directory ...
 	 */
-	error = init_private_file(&file, dentry, FMODE_READ);
+	error = open_private_file(&file, dentry, FMODE_READ);
 	if (error)
 		goto out;
 	error = -EINVAL;
@@ -381,8 +381,7 @@
 	}
 
 out_close:
-	if (file.f_op->release)
-		file.f_op->release(dir, &file);
+	close_private_file(&file);
 out:
 	return error;
 }
===== fs/nfsd/vfs.c 1.55 vs edited =====
--- 1.55/fs/nfsd/vfs.c	Fri Jan 10 20:00:12 2003
+++ edited/fs/nfsd/vfs.c	Wed Jan 15 09:39:35 2003
@@ -468,7 +468,7 @@
 		DQUOT_INIT(inode);
 	}
 
-	err = init_private_file(filp, dentry, mode);
+	err = open_private_file(filp, dentry, mode);
 	if (!err) {
 		filp->f_flags = flags;
 		filp->f_vfsmnt = fhp->fh_export->ex_mnt;
@@ -491,8 +491,7 @@
 	struct dentry	*dentry = filp->f_dentry;
 	struct inode	*inode = dentry->d_inode;
 
-	if (filp->f_op->release)
-		filp->f_op->release(inode, filp);
+	close_private_file(filp);
 	if (filp->f_mode & FMODE_WRITE)
 		put_write_access(inode);
 }
===== include/linux/fs.h 1.210 vs edited =====
--- 1.210/include/linux/fs.h	Wed Jan  8 15:37:23 2003
+++ edited/include/linux/fs.h	Wed Jan 15 09:41:28 2003
@@ -492,7 +492,10 @@
 #define get_file(x)	atomic_inc(&(x)->f_count)
 #define file_count(x)	atomic_read(&(x)->f_count)
 
-extern int init_private_file(struct file *, struct dentry *, int);
+/* Initialize and open a private file and allocate its security structure. */
+extern int open_private_file(struct file *, struct dentry *, int);
+/* Release a private file and free its security structure. */
+extern void close_private_file(struct file *file);
 
 #define	MAX_NON_LFS	((1UL<<31) - 1)
 
===== kernel/ksyms.c 1.178 vs edited =====
--- 1.178/kernel/ksyms.c	Mon Jan 13 04:24:04 2003
+++ edited/kernel/ksyms.c	Wed Jan 15 09:41:47 2003
@@ -179,7 +179,8 @@
 EXPORT_SYMBOL(end_buffer_io_sync);
 EXPORT_SYMBOL(__mark_inode_dirty);
 EXPORT_SYMBOL(get_empty_filp);
-EXPORT_SYMBOL(init_private_file);
+EXPORT_SYMBOL(open_private_file);
+EXPORT_SYMBOL(close_private_file);
 EXPORT_SYMBOL(filp_open);
 EXPORT_SYMBOL(filp_close);
 EXPORT_SYMBOL(put_filp);





--
Stephen Smalley, NSA
sds@epoch.ncsc.mil

Re: [RFC] Changes to the LSM file-related hooks for 2.5.58

[Andi Kleen]

"Stephen D. Smalley" <sds@epoch.ncsc.mil> writes:
> 
> 3) Add a security_file_alloc call to init_private_file and create a
> release_private_file function to encapsulate release of private file
> structures.  These changes ensure that security structures for private
> files will be allocated and freed appropriately.

Adding release_private_file requires fixing all code that uses 
init_private_file (including possible third party code). Otherwise
you have some subtle leak. It would better to rename init_private_file to
some other name and add appropiate comments so that this can be catched 
easily at compile time.

-Andi