Re: [PATCH v5 05/21] ima: move ima_file_free before releasing thefile

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@xxxxxxxxxxxxxxxxxx):
> On Thu, 2011-05-19 at 17:06 -0500, Serge E. Hallyn wrote:
> > Quoting Mimi Zohar (zohar@xxxxxxxxxxxxxxxxxx):
> > > Integrity appraisal measures files on file_free and stores the file
> > > measurement as an xattr.  Measure the file before releasing it.
> > 
> > Can you put a bit more in the commit msg about why?  What's magic
> > about the fs-specific release function?
> 
> ima_file_free(), called on __fput(), currently flags files that have
> changed, so that the file is re-measured.  However, for appraising a
> files's integrity, flagging the file is not enough.  The file's
> security.ima xattr must be updated to reflect any changes.  This patch
> moves releasing the file to after calculating the new file hash.

Sorry if I'm being dense, but I still don't understand (even though
apparently I used to :)  why the fs release is magic here.  The
dropping of the writelock comes later, so no file will be able to
open the file for execute or write until that point, meaning that
won't be happening without a re-measure with or without this patch.

So you must be thinking something about general opens(), but I
don't believe that file_operations->release makes a difference to
another tasks's ability to open the file, nor to the writing out
of changed contents.

security_file_free() doesn't appear to be hooked by ima or evm,
and if a security module changes its security.X xattr you'll
end up re-measuring the xattrs anyway.

So I'm still missing something, sorry  :)

-serge

> > > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxx>
> > > Acked-by: Serge Hallyn <serge.hallyn@xxxxxxxxxx>
> > > ---
> > >  fs/file_table.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/fs/file_table.c b/fs/file_table.c
> > > index 01e4c1e..33f54a1 100644
> > > --- a/fs/file_table.c
> > > +++ b/fs/file_table.c
> > > @@ -243,10 +243,10 @@ static void __fput(struct file *file)
> > >  		if (file->f_op && file->f_op->fasync)
> > >  			file->f_op->fasync(-1, file, 0);
> > >  	}
> > > +	ima_file_free(file);
> > >  	if (file->f_op && file->f_op->release)
> > >  		file->f_op->release(inode, file);
> > >  	security_file_free(file);
> > > -	ima_file_free(file);
> > >  	if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
> > >  		     !(file->f_mode & FMODE_PATH))) {
> > >  		cdev_put(inode->i_cdev);
> > > -- 
> 
--
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/