Re: [PATCH 3/4] integrity: Linux Integrity Module(LIM)

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Mimi Zohar (zohar@us.ibm.com):
> Christoph Hellwig <hch@infradead.org> wrote on 08/09/2008 02:53:40 PM:
> 
> > > +      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;
> > > +      }
> > > +
> > 
> > Please factor this and the lsm failure case out into a single
> > out_free_inode goto label.
> 
> ok
> 
> > >  int vfs_permission(struct nameidata *nd, int mask)
> > >  {
> > > -   return inode_permission(nd->path.dentry->d_inode, mask);
> > > +   int retval;
> > > +
> > > +   retval = inode_permission(nd->path.dentry->d_inode, mask);
> > > +   if (retval)
> > > +      return retval;
> > > +   return integrity_inode_permission(NULL, &nd->path,
> > > +                 mask & (MAY_READ | MAY_WRITE |
> > > +                    MAY_EXEC));
> > >  }
> > > 
> > >  /**
> > > @@ -306,7 +314,14 @@ int vfs_permission(struct nameidata *nd,
> > >   */
> > >  int file_permission(struct file *file, int mask)
> > >  {
> > > -   return inode_permission(file->f_path.dentry->d_inode, mask);
> > > +   int retval;
> > > +
> > > +   retval = inode_permission(file->f_path.dentry->d_inode, mask);
> > > +   if (retval)
> > > +      return retval;
> > > +   return integrity_inode_permission(file, NULL,
> > > +                 mask & (MAY_READ | MAY_WRITE |
> > > +                    MAY_EXEC));
> > 
> > Please put your hook into inode_permission.  Note that in inode
> > permission and lots of callers there is no path available so don't pass
> > it.  Please pass the full MAY_FOO mask for new interfaces and do
> > filtering that won't break if new ones are introduced.
> 
> We started out with the integrity_inode_permission() hook call in 
> inode_permission(), but because of the removal of the nameidata 
> parameter in the last merge, based on discussions
> http://marc.info/?l=linux-security-module&m=121797845308246&w=2,
> the call to integrity_inode_permission() was moved up to the caller,
> where either a file or path are available.  Any suggestions?

Mimi, can you explain exactly (and concisely) what you are doing with
the pathname?

> > > +#ifdef CONFIG_INTEGRITY
> > > +   void         *i_integrity;
> > > +#endif
> > 
> > Sorry, but I don't think we can bloat the inode even further for this.
> 
> The original version of IMA was LSM based, using i_security. Based
> on discussions on the LSM mailing list, it was decided that the LSM hooks
> were meant only for access control.  During the same time frame, there 
> was a lot of work done in stacking LSM modules and i_security, but that
> approach was dropped. It was suggested that we define a separate set of
> hooks for integrity, which this patch set provides. Caching integrity 
> results is an important aspect. Any suggestions in lieu of defining 
> i_integrity?

The i_integrity is only bloating the inode if LIM is enabled.  Surely
that beats having LIM define its own hash table and locking to track
integrity labels on inodes?  Do you have another suggestion?

Or is the concern about having more #ifdefs in the struct inode
definition?

-serge