In-Reply-To: <20080809185340.GC22905@infradead.org>
References: <20080808184349.999902616@linux.vnet.ibm.com> <1218221761.4444.13.camel@localhost.localdomain> <20080809185340.GC22905@infradead.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org,
       Al Viro <viro@ZenIV.linux.org.uk>, Stephen Smalley <sds@tycho.nsa.gov>,
       James Morris <jmorris@namei.org>,
       Randy Dunlap <randy.dunlap@oracle.com>, safford@watson.ibm.com,
       serue@linux.vnet.ibm.com, sailer@watson.ibm.com,
       Mimi Zohar <zohar@linux.vnet.ibm.com>
MIME-Version: 1.0
Subject: Re: [PATCH 3/4] integrity: Linux Integrity Module(LIM)
Message-ID: <OF0AEBDCA2.1E27B823-ON852574A1.000FEE48-852574A1.004C3185@us.ibm.com>
From: Mimi Zohar <zohar@us.ibm.com>
Date: Sun, 10 Aug 2008 09:52:13 -0400
Content-Type: text/plain; charset="US-ASCII"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3047
Lines: 84

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?
 
> > +#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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/