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

[Mimi Zohar]

Christoph Hellwig <hch@xxxxxxxxxxxxx> 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?
 
> > +#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?

> > +/*
> > + * integrity.h
> 
> don't bother to mention the filename in the top of file comment.
 
ok

Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/