lkml.org 
[lkml]   [2008]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/4] integrity: Linux Integrity Module(LIM)
From
Date
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


\
 
 \ /
  Last update: 2008-08-10 15:55    [W:0.128 / U:2.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site