[RFC][Patch 2/6] integrity: fs hook placement

[RFC][Patch 2/6] integrity: fs hook placement

[Mimi Zohar]

This patch places calls to the new integrity hooks in the appropriate
places in the fs directory. It is not meant in any way to be viewed
as a complete set, but used as a basis for an initial discussion.

Index: linux-2.6.21-rc3-mm2/fs/ext3/xattr_security.c
===================================================================
--- linux-2.6.21-rc3-mm2.orig/fs/ext3/xattr_security.c
+++ linux-2.6.21-rc3-mm2/fs/ext3/xattr_security.c
@@ -10,6 +10,7 @@
 #include <linux/ext3_jbd.h>
 #include <linux/ext3_fs.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include "xattr.h"
 
 static size_t
@@ -58,12 +59,19 @@ ext3_init_security(handle_t *handle, str
 
 	err = security_inode_init_security(inode, dir, &name, &value, &len);
 	if (err) {
+		/* Even if creation of the security xattr fails, must
+		 * indicate this is a new inode. */
+		integrity_inode_init_integrity(inode, dir, NULL, NULL, NULL);
 		if (err == -EOPNOTSUPP)
 			return 0;
 		return err;
 	}
 	err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY,
 				    name, value, len, 0);
+
+	integrity_inode_init_integrity(inode, dir, &name, &value, &len);
+	err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY,
+				    name, value, len, 0);
 	kfree(name);
 	kfree(value);
 	return err;
Index: linux-2.6.21-rc3-mm2/include/linux/fs.h
===================================================================
--- linux-2.6.21-rc3-mm2.orig/include/linux/fs.h
+++ linux-2.6.21-rc3-mm2/include/linux/fs.h
@@ -591,6 +591,9 @@ struct inode {
 #ifdef CONFIG_SECURITY
 	void			*i_security;
 #endif
+#ifdef CONFIG_INTEGRITY
+	void			*i_integrity;
+#endif
 	void			*i_private; /* fs or device private pointer */
 };
 
Index: linux-2.6.21-rc3-mm2/fs/dcache.c
===================================================================
--- linux-2.6.21-rc3-mm2.orig/fs/dcache.c
+++ linux-2.6.21-rc3-mm2/fs/dcache.c
@@ -29,6 +29,7 @@
 #include <linux/file.h>
 #include <asm/uaccess.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/seqlock.h>
 #include <linux/swap.h>
 #include <linux/bootmem.h>
@@ -986,6 +987,7 @@ void d_instantiate(struct dentry *entry,
 	entry->d_inode = inode;
 	fsnotify_d_instantiate(entry, inode);
 	spin_unlock(&dcache_lock);
+	integrity_d_instantiate(entry, inode);
 	security_d_instantiate(entry, inode);
 }
 
@@ -1050,6 +1052,7 @@ struct dentry *d_instantiate_unique(stru
 	spin_unlock(&dcache_lock);
 
 	if (!result) {
+		integrity_d_instantiate(entry, inode);
 		security_d_instantiate(entry, inode);
 		return NULL;
 	}
@@ -1187,6 +1190,7 @@ struct dentry *d_splice_alias(struct ino
 			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
 			fsnotify_d_instantiate(new, inode);
 			spin_unlock(&dcache_lock);
+			integrity_d_instantiate(new, inode);
 			security_d_instantiate(new, inode);
 			d_rehash(dentry);
 			d_move(new, dentry);
@@ -1197,6 +1201,7 @@ struct dentry *d_splice_alias(struct ino
 			dentry->d_inode = inode;
 			fsnotify_d_instantiate(dentry, inode);
 			spin_unlock(&dcache_lock);
+			integrity_d_instantiate(dentry, inode);
 			security_d_instantiate(dentry, inode);
 			d_rehash(dentry);
 		}
@@ -1748,6 +1753,7 @@ found:
 	spin_unlock(&dcache_lock);
 out_nolock:
 	if (actual == dentry) {
+		integrity_d_instantiate(dentry, inode);
 		security_d_instantiate(dentry, inode);
 		return NULL;
 	}
Index: linux-2.6.21-rc3-mm2/fs/file_table.c
===================================================================
--- linux-2.6.21-rc3-mm2.orig/fs/file_table.c
+++ linux-2.6.21-rc3-mm2/fs/file_table.c
@@ -13,6 +13,7 @@
 #include <linux/smp_lock.h>
 #include <linux/fs.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/eventpoll.h>
 #include <linux/rcupdate.h>
 #include <linux/mount.h>
@@ -169,6 +170,7 @@ void fastcall __fput(struct file *file)
 	if (file->f_op && file->f_op->release)
 		file->f_op->release(inode, file);
 	security_file_free(file);
+	integrity_file_free(file);
 	if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL))
 		cdev_put(inode->i_cdev);
 	fops_put(file->f_op);
@@ -240,6 +242,7 @@ void put_filp(struct file *file)
 {
 	if (atomic_dec_and_test(&file->f_count)) {
 		security_file_free(file);
+		integrity_file_free(file);
 		file_kill(file);
 		file_free(file);
 	}
Index: linux-2.6.21-rc3-mm2/fs/inode.c
===================================================================
--- linux-2.6.21-rc3-mm2.orig/fs/inode.c
+++ linux-2.6.21-rc3-mm2/fs/inode.c
@@ -17,6 +17,7 @@
 #include <linux/hash.h>
 #include <linux/swap.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/pagemap.h>
 #include <linux/cdev.h>
 #include <linux/bootmem.h>
@@ -142,6 +143,14 @@ static struct inode *alloc_inode(struct 
 			return NULL;
 		}
 
+		if (integrity_inode_alloc(inode)) {
+			if (inode->i_sb->s_op->destroy_inode)
+				inode->i_sb->s_op->destroy_inode(inode);
+			else
+				kmem_cache_free(inode_cachep, (inode));
+			return NULL;
+		}
+
 		mapping->a_ops = &empty_aops;
  		mapping->host = inode;
 		mapping->flags = 0;
@@ -172,6 +181,7 @@ void destroy_inode(struct inode *inode) 
 {
 	BUG_ON(inode_has_buffers(inode));
 	security_inode_free(inode);
+	integrity_inode_free(inode);
 	if (inode->i_sb->s_op->destroy_inode)
 		inode->i_sb->s_op->destroy_inode(inode);
 	else
Index: linux-2.6.21-rc3-mm2/fs/xattr.c
===================================================================
--- linux-2.6.21-rc3-mm2.orig/fs/xattr.c
+++ linux-2.6.21-rc3-mm2/fs/xattr.c
@@ -14,6 +14,7 @@
 #include <linux/xattr.h>
 #include <linux/namei.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/syscalls.h>
 #include <linux/module.h>
 #include <linux/fsnotify.h>
@@ -84,11 +85,17 @@ vfs_setxattr(struct dentry *dentry, char
 	error = security_inode_setxattr(dentry, name, value, size, flags);
 	if (error)
 		goto out;
+
+	error = integrity_inode_setxattr(dentry, name, value, size, flags);
+	if (error)
+		goto out;
+
 	error = -EOPNOTSUPP;
 	if (inode->i_op->setxattr) {
 		error = inode->i_op->setxattr(dentry, name, value, size, flags);
 		if (!error) {
 			fsnotify_xattr(dentry);
+			integrity_inode_post_setxattr(dentry, name);
 			security_inode_post_setxattr(dentry, name, value,
 						     size, flags);
 		}
@@ -181,6 +188,8 @@ vfs_removexattr(struct dentry *dentry, c
 
 	mutex_lock(&inode->i_mutex);
 	error = inode->i_op->removexattr(dentry, name);
+	if (!error)
+		integrity_inode_post_setxattr(dentry, name);
 	mutex_unlock(&inode->i_mutex);
 
 	if (!error)

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Chris Wright]

* Mimi Zohar (zohar@linux.vnet.ibm.com) wrote:
> +	integrity_d_instantiate(entry, inode);
>  	security_d_instantiate(entry, inode);

> +		integrity_d_instantiate(entry, inode);
>  		security_d_instantiate(entry, inode);

>  			spin_unlock(&dcache_lock);
> +			integrity_d_instantiate(new, inode);
>  			security_d_instantiate(new, inode);
>  			d_rehash(dentry);

>  			d_move(new, dentry);
> +			integrity_d_instantiate(dentry, inode);
>  			security_d_instantiate(dentry, inode);
>  			d_rehash(dentry);

>  	if (actual == dentry) {
> +		integrity_d_instantiate(dentry, inode);
>  		security_d_instantiate(dentry, inode);


>  		file->f_op->release(inode, file);
>  	security_file_free(file);
> +	integrity_file_free(file);


>  		security_file_free(file);
> +		integrity_file_free(file);


> 		if (security_inode_alloc(inode)) {
...
> +		if (integrity_inode_alloc(inode)) {
> +			if (inode->i_sb->s_op->destroy_inode)
> +				inode->i_sb->s_op->destroy_inode(inode);

>  	security_inode_free(inode);
> +	integrity_inode_free(inode);

>  	error = security_inode_setxattr(dentry, name, value, size, flags);
>  	if (error)
>  		goto out;
> +
> +	error = integrity_inode_setxattr(dentry, name, value, size, flags);

>  			fsnotify_xattr(dentry);
> +			integrity_inode_post_setxattr(dentry, name);
>  			security_inode_post_setxattr(dentry, name, value,

I know there's some slightly different goals, but this just doesn't make
sense.  Need to get back to defining and expressing just the differences.

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Serge E. Hallyn]

Quoting Chris Wright (chrisw@sous-sol.org):
> * Mimi Zohar (zohar@linux.vnet.ibm.com) wrote:
> > +	integrity_d_instantiate(entry, inode);
> >  	security_d_instantiate(entry, inode);
> 
> > +		integrity_d_instantiate(entry, inode);
> >  		security_d_instantiate(entry, inode);
> 
> >  			spin_unlock(&dcache_lock);
> > +			integrity_d_instantiate(new, inode);
> >  			security_d_instantiate(new, inode);
> >  			d_rehash(dentry);
> 
> >  			d_move(new, dentry);
> > +			integrity_d_instantiate(dentry, inode);
> >  			security_d_instantiate(dentry, inode);
> >  			d_rehash(dentry);
> 
> >  	if (actual == dentry) {
> > +		integrity_d_instantiate(dentry, inode);
> >  		security_d_instantiate(dentry, inode);
> 
> 
> >  		file->f_op->release(inode, file);
> >  	security_file_free(file);
> > +	integrity_file_free(file);
> 
> 
> >  		security_file_free(file);
> > +		integrity_file_free(file);
> 
> 
> > 		if (security_inode_alloc(inode)) {
> ...
> > +		if (integrity_inode_alloc(inode)) {
> > +			if (inode->i_sb->s_op->destroy_inode)
> > +				inode->i_sb->s_op->destroy_inode(inode);
> 
> >  	security_inode_free(inode);
> > +	integrity_inode_free(inode);
> 
> >  	error = security_inode_setxattr(dentry, name, value, size, flags);
> >  	if (error)
> >  		goto out;
> > +
> > +	error = integrity_inode_setxattr(dentry, name, value, size, flags);
> 
> >  			fsnotify_xattr(dentry);
> > +			integrity_inode_post_setxattr(dentry, name);
> >  			security_inode_post_setxattr(dentry, name, value,
> 
> I know there's some slightly different goals, but this just doesn't make
> sense.  Need to get back to defining and expressing just the differences.

Are you objecting only to the duplication at the callsites, so that an
fsnotify-type of consolidation of security and integrity hooks would be
ok?  Or are you complaining that the security_inode_setxattr and
integrity_inode_setxattr hooks are too similar anyway, and integrity
modules should just use some lsm hooks for anything which will be
authoritative?

(I could see an argument that integirty subsystem should be purely for
measuring and hence its hooks should never return a value.  Only hitch
there is that if integrity subsystem hits ENOMEM it should be able to
refuse the action...)

-serge

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> Are you objecting only to the duplication at the callsites, so that an
> fsnotify-type of consolidation of security and integrity hooks would be
> ok?  Or are you complaining that the security_inode_setxattr and
> integrity_inode_setxattr hooks are too similar anyway, and integrity
> modules should just use some lsm hooks for anything which will be
> authoritative?

It's duplication of callsites with many identical implementations
that's the problem.

> (I could see an argument that integirty subsystem should be purely for
> measuring and hence its hooks should never return a value.  Only hitch
> there is that if integrity subsystem hits ENOMEM it should be able to
> refuse the action...)

Right, that's what I was expecting to see, just the measurement
infrastructure.

thanks,
-chris

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Serge E. Hallyn]

Quoting Chris Wright (chrisw@sous-sol.org):
> * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > Are you objecting only to the duplication at the callsites, so that an
> > fsnotify-type of consolidation of security and integrity hooks would be
> > ok?  Or are you complaining that the security_inode_setxattr and
> > integrity_inode_setxattr hooks are too similar anyway, and integrity
> > modules should just use some lsm hooks for anything which will be
> > authoritative?
> 
> It's duplication of callsites with many identical implementations
> that's the problem.

Yes it's ugly...

But I guess it gets a point across :)

> > (I could see an argument that integirty subsystem should be purely for
> > measuring and hence its hooks should never return a value.  Only hitch
> > there is that if integrity subsystem hits ENOMEM it should be able to
> > refuse the action...)
> 
> Right, that's what I was expecting to see, just the measurement
> infrastructure.

So what you are saying is EVM would stay an LSM, with a cooperating
integrity subsystem *just* doing measurements?

That's kind of what i was expecting too, however that doesn't fit as
well with the idea that an integrity subsystem prevents the need for lsm
stacking.  I think the idea was that evm would still be able to enforce
integrity of selinux xattrs without it stack with selinux.  So I can see
where this approach came from.

-serge

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Dmitriy Monakhov]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> This patch places calls to the new integrity hooks in the appropriate
> places in the fs directory. It is not meant in any way to be viewed
> as a complete set, but used as a basis for an initial discussion.
>
> Index: linux-2.6.21-rc3-mm2/fs/ext3/xattr_security.c
> ===================================================================
> --- linux-2.6.21-rc3-mm2.orig/fs/ext3/xattr_security.c
> +++ linux-2.6.21-rc3-mm2/fs/ext3/xattr_security.c
> @@ -10,6 +10,7 @@
>  #include <linux/ext3_jbd.h>
>  #include <linux/ext3_fs.h>
>  #include <linux/security.h>
> +#include <linux/integrity.h>
>  #include "xattr.h"
>  
>  static size_t
> @@ -58,12 +59,19 @@ ext3_init_security(handle_t *handle, str
>  
>  	err = security_inode_init_security(inode, dir, &name, &value, &len);
>  	if (err) {
> +		/* Even if creation of the security xattr fails, must
> +		 * indicate this is a new inode. */
> +		integrity_inode_init_integrity(inode, dir, NULL, NULL, NULL);
>  		if (err == -EOPNOTSUPP)
>  			return 0;
>  		return err;
>  	}
<<<< block begin
>  	err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY,
>  				    name, value, len, 0);
> +
> +	integrity_inode_init_integrity(inode, dir, &name, &value, &len);
> +	err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY,
> +				    name, value, len, 0);
<<<< block end
May be i've missed something, but i don't get last block.
Why you call ext3_xattr_set_handle() twise?, 
or you just mistyped and  it has to look like this:
<<<<block_begin

+	integrity_inode_init_integrity(inode, dir, &name, &value, &len);
	err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY,
				    name, value, len, 0);
<<<block_end
>  	kfree(name);
>  	kfree(value);
>  	return err;
> Index: linux-2.6.21-rc3-mm2/include/linux/fs.h
> ===================================================================
> --- linux-2.6.21-rc3-mm2.orig/include/linux/fs.h
> +++ linux-2.6.21-rc3-mm2/include/linux/fs.h
> @@ -591,6 +591,9 @@ struct inode {
>  #ifdef CONFIG_SECURITY
>  	void			*i_security;
>  #endif
> +#ifdef CONFIG_INTEGRITY
> +	void			*i_integrity;
> +#endif
>  	void			*i_private; /* fs or device private pointer */
>  };
>  
> Index: linux-2.6.21-rc3-mm2/fs/dcache.c
> ===================================================================
> --- linux-2.6.21-rc3-mm2.orig/fs/dcache.c
> +++ linux-2.6.21-rc3-mm2/fs/dcache.c
> @@ -29,6 +29,7 @@
>  #include <linux/file.h>
>  #include <asm/uaccess.h>
>  #include <linux/security.h>
> +#include <linux/integrity.h>
>  #include <linux/seqlock.h>
>  #include <linux/swap.h>
>  #include <linux/bootmem.h>
> @@ -986,6 +987,7 @@ void d_instantiate(struct dentry *entry,
>  	entry->d_inode = inode;
>  	fsnotify_d_instantiate(entry, inode);
>  	spin_unlock(&dcache_lock);
> +	integrity_d_instantiate(entry, inode);
>  	security_d_instantiate(entry, inode);
>  }
>  
> @@ -1050,6 +1052,7 @@ struct dentry *d_instantiate_unique(stru
>  	spin_unlock(&dcache_lock);
>  
>  	if (!result) {
> +		integrity_d_instantiate(entry, inode);
>  		security_d_instantiate(entry, inode);
>  		return NULL;
>  	}
> @@ -1187,6 +1190,7 @@ struct dentry *d_splice_alias(struct ino
>  			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
>  			fsnotify_d_instantiate(new, inode);
>  			spin_unlock(&dcache_lock);
> +			integrity_d_instantiate(new, inode);
>  			security_d_instantiate(new, inode);
>  			d_rehash(dentry);
>  			d_move(new, dentry);
> @@ -1197,6 +1201,7 @@ struct dentry *d_splice_alias(struct ino
>  			dentry->d_inode = inode;
>  			fsnotify_d_instantiate(dentry, inode);
>  			spin_unlock(&dcache_lock);
> +			integrity_d_instantiate(dentry, inode);
>  			security_d_instantiate(dentry, inode);
>  			d_rehash(dentry);
>  		}
> @@ -1748,6 +1753,7 @@ found:
>  	spin_unlock(&dcache_lock);
>  out_nolock:
>  	if (actual == dentry) {
> +		integrity_d_instantiate(dentry, inode);
>  		security_d_instantiate(dentry, inode);
>  		return NULL;
>  	}
> Index: linux-2.6.21-rc3-mm2/fs/file_table.c
> ===================================================================
> --- linux-2.6.21-rc3-mm2.orig/fs/file_table.c
> +++ linux-2.6.21-rc3-mm2/fs/file_table.c
> @@ -13,6 +13,7 @@
>  #include <linux/smp_lock.h>
>  #include <linux/fs.h>
>  #include <linux/security.h>
> +#include <linux/integrity.h>
>  #include <linux/eventpoll.h>
>  #include <linux/rcupdate.h>
>  #include <linux/mount.h>
> @@ -169,6 +170,7 @@ void fastcall __fput(struct file *file)
>  	if (file->f_op && file->f_op->release)
>  		file->f_op->release(inode, file);
>  	security_file_free(file);
> +	integrity_file_free(file);
>  	if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL))
>  		cdev_put(inode->i_cdev);
>  	fops_put(file->f_op);
> @@ -240,6 +242,7 @@ void put_filp(struct file *file)
>  {
>  	if (atomic_dec_and_test(&file->f_count)) {
>  		security_file_free(file);
> +		integrity_file_free(file);
>  		file_kill(file);
>  		file_free(file);
>  	}
> Index: linux-2.6.21-rc3-mm2/fs/inode.c
> ===================================================================
> --- linux-2.6.21-rc3-mm2.orig/fs/inode.c
> +++ linux-2.6.21-rc3-mm2/fs/inode.c
> @@ -17,6 +17,7 @@
>  #include <linux/hash.h>
>  #include <linux/swap.h>
>  #include <linux/security.h>
> +#include <linux/integrity.h>
>  #include <linux/pagemap.h>
>  #include <linux/cdev.h>
>  #include <linux/bootmem.h>
> @@ -142,6 +143,14 @@ static struct inode *alloc_inode(struct 
>  			return NULL;
>  		}
>  
> +		if (integrity_inode_alloc(inode)) {
> +			if (inode->i_sb->s_op->destroy_inode)
> +				inode->i_sb->s_op->destroy_inode(inode);
> +			else
> +				kmem_cache_free(inode_cachep, (inode));
> +			return NULL;
> +		}
> +
>  		mapping->a_ops = &empty_aops;
>   		mapping->host = inode;
>  		mapping->flags = 0;
> @@ -172,6 +181,7 @@ void destroy_inode(struct inode *inode) 
>  {
>  	BUG_ON(inode_has_buffers(inode));
>  	security_inode_free(inode);
> +	integrity_inode_free(inode);
>  	if (inode->i_sb->s_op->destroy_inode)
>  		inode->i_sb->s_op->destroy_inode(inode);
>  	else
> Index: linux-2.6.21-rc3-mm2/fs/xattr.c
> ===================================================================
> --- linux-2.6.21-rc3-mm2.orig/fs/xattr.c
> +++ linux-2.6.21-rc3-mm2/fs/xattr.c
> @@ -14,6 +14,7 @@
>  #include <linux/xattr.h>
>  #include <linux/namei.h>
>  #include <linux/security.h>
> +#include <linux/integrity.h>
>  #include <linux/syscalls.h>
>  #include <linux/module.h>
>  #include <linux/fsnotify.h>
> @@ -84,11 +85,17 @@ vfs_setxattr(struct dentry *dentry, char
>  	error = security_inode_setxattr(dentry, name, value, size, flags);
>  	if (error)
>  		goto out;
> +
> +	error = integrity_inode_setxattr(dentry, name, value, size, flags);
> +	if (error)
> +		goto out;
> +
>  	error = -EOPNOTSUPP;
>  	if (inode->i_op->setxattr) {
>  		error = inode->i_op->setxattr(dentry, name, value, size, flags);
>  		if (!error) {
>  			fsnotify_xattr(dentry);
> +			integrity_inode_post_setxattr(dentry, name);
>  			security_inode_post_setxattr(dentry, name, value,
>  						     size, flags);
>  		}
> @@ -181,6 +188,8 @@ vfs_removexattr(struct dentry *dentry, c
>  
>  	mutex_lock(&inode->i_mutex);
>  	error = inode->i_op->removexattr(dentry, name);
> +	if (!error)
> +		integrity_inode_post_setxattr(dentry, name);
>  	mutex_unlock(&inode->i_mutex);
>  
>  	if (!error)
>
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Mimi Zohar]

On Thu, 2007-03-08 at 09:40 -0800, Chris Wright wrote:
> * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > Are you objecting only to the duplication at the callsites, so that an
> > fsnotify-type of consolidation of security and integrity hooks would be
> > ok?  Or are you complaining that the security_inode_setxattr and
> > integrity_inode_setxattr hooks are too similar anyway, and integrity
> > modules should just use some lsm hooks for anything which will be
> > authoritative?
> 
> It's duplication of callsites with many identical implementations
> that's the problem.
>
> > (I could see an argument that integirty subsystem should be purely for
> > measuring and hence its hooks should never return a value.  Only hitch
> > there is that if integrity subsystem hits ENOMEM it should be able to
> > refuse the action...)
> 
> Right, that's what I was expecting to see, just the measurement
> infrastructure.

There are a total of 10 Linux Integrity Module(LIM) hooks. Seven of
which parallel the LSM hooks, out of the ~150 LSM hooks.  3 of the LIM
hooks are for initializing, allocating, and freeing the inode-
>i_integrity, used for caching integrity information.  As the integrity
information is stored as extended attributes, 2 hooks are for catching
changes to the extended attributes, one is for updating the extended
attributes when the file closes, and d_instantiate is used for
initialization.  Is this excessive?  How else would you design
integrity, without using the LSM hooks?

Mimi Zohar

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Mimi Zohar]

On Thu, 2007-03-08 at 21:16 +0300, Dmitriy Monakhov wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > This patch places calls to the new integrity hooks in the appropriate
> > places in the fs directory. It is not meant in any way to be viewed
> > as a complete set, but used as a basis for an initial discussion.
> >
> > Index: linux-2.6.21-rc3-mm2/fs/ext3/xattr_security.c
> > ===================================================================
> > --- linux-2.6.21-rc3-mm2.orig/fs/ext3/xattr_security.c
> > +++ linux-2.6.21-rc3-mm2/fs/ext3/xattr_security.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/ext3_jbd.h>
> >  #include <linux/ext3_fs.h>
> >  #include <linux/security.h>
> > +#include <linux/integrity.h>
> >  #include "xattr.h"
> >  
> >  static size_t
> > @@ -58,12 +59,19 @@ ext3_init_security(handle_t *handle, str
> >  
> >  	err = security_inode_init_security(inode, dir, &name, &value, &len);
> >  	if (err) {
> > +		/* Even if creation of the security xattr fails, must
> > +		 * indicate this is a new inode. */
> > +		integrity_inode_init_integrity(inode, dir, NULL, NULL, NULL);
> >  		if (err == -EOPNOTSUPP)
> >  			return 0;
> >  		return err;
> >  	}
> <<<< block begin
> >  	err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY,
> >  				    name, value, len, 0);
> > +
> > +	integrity_inode_init_integrity(inode, dir, &name, &value, &len);
> > +	err = ext3_xattr_set_handle(handle, inode, EXT3_XATTR_INDEX_SECURITY,
> > +				    name, value, len, 0);
> <<<< block end
> May be i've missed something, but i don't get last block.
> Why you call ext3_xattr_set_handle() twise?, 
> or you just mistyped and  it has to look like this:
> <<<<block_begin

Both security_inode_init_security and integrity_inode_init_integrity
create extended attributes. The first ext3_xattr_set_handle creates the
security extended attribute and the second one creates the integrity
extended attribute.  In the case of EVM, EVM calculates an HMAC on a set
of extended attributes, possibly including the initial security extended
attribute.

Mimi Zohar

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Stephen Smalley]

On Thu, 2007-03-08 at 12:01 -0600, Serge E. Hallyn wrote:
> Quoting Chris Wright (chrisw@sous-sol.org):
> > * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > > Are you objecting only to the duplication at the callsites, so that an
> > > fsnotify-type of consolidation of security and integrity hooks would be
> > > ok?  Or are you complaining that the security_inode_setxattr and
> > > integrity_inode_setxattr hooks are too similar anyway, and integrity
> > > modules should just use some lsm hooks for anything which will be
> > > authoritative?
> > 
> > It's duplication of callsites with many identical implementations
> > that's the problem.
> 
> Yes it's ugly...
> 
> But I guess it gets a point across :)
> 
> > > (I could see an argument that integirty subsystem should be purely for
> > > measuring and hence its hooks should never return a value.  Only hitch
> > > there is that if integrity subsystem hits ENOMEM it should be able to
> > > refuse the action...)
> > 
> > Right, that's what I was expecting to see, just the measurement
> > infrastructure.
> 
> So what you are saying is EVM would stay an LSM, with a cooperating
> integrity subsystem *just* doing measurements?
> 
> That's kind of what i was expecting too, however that doesn't fit as
> well with the idea that an integrity subsystem prevents the need for lsm
> stacking.  I think the idea was that evm would still be able to enforce
> integrity of selinux xattrs without it stack with selinux.  So I can see
> where this approach came from.

The enforcement mechanism should be directly integrated into SELinux,
not stacked as a separate module.

-- 
Stephen Smalley
National Security Agency

Re: [RFC][Patch 2/6] integrity: fs hook placement

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> On Thu, 2007-03-08 at 12:01 -0600, Serge E. Hallyn wrote:
> > Quoting Chris Wright (chrisw@sous-sol.org):
> > > * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > > > Are you objecting only to the duplication at the callsites, so that an
> > > > fsnotify-type of consolidation of security and integrity hooks would be
> > > > ok?  Or are you complaining that the security_inode_setxattr and
> > > > integrity_inode_setxattr hooks are too similar anyway, and integrity
> > > > modules should just use some lsm hooks for anything which will be
> > > > authoritative?
> > > 
> > > It's duplication of callsites with many identical implementations
> > > that's the problem.
> > 
> > Yes it's ugly...
> > 
> > But I guess it gets a point across :)
> > 
> > > > (I could see an argument that integirty subsystem should be purely for
> > > > measuring and hence its hooks should never return a value.  Only hitch
> > > > there is that if integrity subsystem hits ENOMEM it should be able to
> > > > refuse the action...)
> > > 
> > > Right, that's what I was expecting to see, just the measurement
> > > infrastructure.
> > 
> > So what you are saying is EVM would stay an LSM, with a cooperating
> > integrity subsystem *just* doing measurements?
> > 
> > That's kind of what i was expecting too, however that doesn't fit as
> > well with the idea that an integrity subsystem prevents the need for lsm
> > stacking.  I think the idea was that evm would still be able to enforce
> > integrity of selinux xattrs without it stack with selinux.  So I can see
> > where this approach came from.
> 
> The enforcement mechanism should be directly integrated into SELinux,
> not stacked as a separate module.

And a big plus of splitting it up as I was describing is that anyone
who wanted could make use of the measurement module to add such
functionality to SELinux, as an alternative to the EVM LSM module.

-serge