Re: [PATCH] ima: prevent dead lock when a file is opened for directio

[Al Viro]

On Wed, Feb 27, 2013 at 11:21:15AM +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 27, 2013 at 1:22 AM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> > On Tue, 2013-02-26 at 20:34 +0000, Al Viro wrote:
> >> On Tue, Feb 26, 2013 at 02:32:08PM -0500, Mimi Zohar wrote:
> >> > Before anything gets access to the file, the file needs to be measured,
> >> > appraised, and/or audited, based on policy.  If IMA-appraisal is enabled
> >> > and the file is in policy, we enforce local file integrity and return
> >> > -EINVAL on failure, similar to LSMs.
> >> >
> >> > Appraising the file is a two step process.  Before appraising the file
> >> > data's integrity, we verify the integrity of the file metadata. Included
> >> > in the 'security.evm' calculation is the ino, generation, uid, gid,
> >> > mode, uuid, and the security xattrs.  'security.ima' contains the file
> >> > data hash or a signature based on the hash.
> >> >
> >> > The i_mutex is held when making file metadata changes (eg. xattrs,
> >> > chmod, ...).  We hold the i_mutex through the entire verification,
> >> > preventing the file data/metadata from changing.
> >>
> >> ->i_mutex is *not* guaranteed to prevent file data changes.  It does
> >> cover metadata, but that's it.  ->write() is not required to take it.
> >> Note, BTW, that as soon as you've dropped ->i_mutex, the metadata can
> >> be changed by somebody else.
> >
> > Any time file metadata included in the HMAC is updated, 'security.evm'
> > is updated to reflect the change.  But before 'security.evm' is updated,
> > evm_verify_current_integrity() verifies the existing value.
> >
> >> What do you achieve by holding it over the vfs_read() call?
> >
> > - Before calculating the file hash, verifying it against the digest in
> > 'security.ima' and storing the verification status in the iint, we check
> > the iint to see whether it was previously verified.  By taking the
> > i_mutex and keeping it, we prevent the file from being hashed multiple
> > times.
> >
> > - Prior to IMA-appraisal, on file close only the iint was updated,
> > reflecting the fact that the file would need to be re-measured and added
> > to the measurement list the next time it was opened.  With
> > IMA-appraisal, on file close, not only do we need to reflect this change
> > in the iint, but we also need to update the 'security.ima' xattr to
> > reflect the new hash value.  Having the iint specific lock caused a
> > lockdep.  In one case, we took the i_mutex followed by the iint lock,
> > while in the other case, the iint lock was taken before the i_mutex.
> >
> >> > I guess I wasn't clear here.  IMA always takes the i_mutex, regardless
> >> > of the O_DIRECT flag.  When a file is opened for read,
> >> > process_measurement() takes the i_mutex and then, if the file was opened
> >> > with the O_DIRECT flag, do_blockdev_direct_IO() attempts to take the
> >> > i_mutex again, causing the lockdep.
> >>
> >> *sigh*
> >> Do you actually disagree with my description of the locking rules you
> >> implicitly rely upon?
> >
> > Obsolutely not!  I misunderstood what you were saying.  The word
> > 'unless' was confusing.
> >
> >>  Suppose wankfs_file_read() happens to grab
> >> ->i_mutex for some reason; without IMA it used to be perfectly legitimate.
> >> With IMA it will deadlock as soon as IMA decides that such file is worth
> >> its attention.  So these days the rule has (silently) become
> >>       * ->read() on a regular file is not allowed to touch ->i_mutex
> >> and with your proposed change it becomes (still undocumented)
> >>       * ->read() on a regular file is not allowed to touch ->i_mutex unless
> >> O_DIRECT is present in file flags at the moment of ->read()
> >> Correct?
> >
> > yes, unfortunately.  What would you suggest?
> >
> 
> The main purpose of taking i_mutex is to ensure that measured content
> of the file (vfs_read) is in sync with extended attribute values.
> 
> If in overall taking a i_mutex before calling vsf_read is
> fundamentally wrong, then one of the solutions is to introduce back
> the usage of IMA specific mutex.
> iint->mutex was removed because it caused dead locking due different
> locking order in different cases.

BTW, speaking of races in there: what's to stop ima_file_mmap() from
racing with rename()?  You have
int ima_file_mmap(struct file *file, unsigned long prot)
{
        if (file && (prot & PROT_EXEC))
                return process_measurement(file, file->f_dentry->d_name.name,
                                           MAY_EXEC, MMAP_CHECK);
        return 0;
}
in there; just what is that ->d_name.name good for?  By the time you get
through calculating hash (which, by definition, may include a considerable
amount of IO), the pointer you have passed might be pointing to anything.

If the name is short, it's kept within dentry and updated by d_move() (not
that I'd seen any exclusion with that in there, while we are at it).  If
the name is too long to fit into struct dentry, it's allocated separately.
See fs/dcache.c:switch_names() for what's being ultimately done on rename();
dname_external() is a predicate telling if we store the name separately.
The normal sequence of events if you rename(something/very_long_file_name,
something_else) is this:
	* we get dentries of both source and target; dentry of source has
->d_name pointing to separately allocated name
	* filesystem is asked to move the sucker; suppose it succeeds
	* we do d_move(source, target)
		* target is unhashed
		* source and target names and pointers to parents are
		  exchanged - now source is where we wanted it to be placed
		  and its old ->d_name is not lost - target->d_name points
		  to it now.  Not for long, though, since...
	* ... we drop references to source and target we had acquired.  And
	  since target had been unhashed, down the drain it goes.  Which
	  frees its separately allocated ->d_name (i.e. what used to be
	  old name of source) and drops the reference to its parent (i.e.
	  what used to be the old parent of source).
Now think what happens if you have found source->d_name.name before rename(2)
and dereferenced it after.

The real question is, what are you using that name for?  Or ima_d_path()
result that normally gets used instead of it, for that matter.  Sure,
in case of ima_d_path() you will have a safe copy of what used to be
the pathname.  What is it good for, though, when the file might've been
moved just as ima_d_path() returns its copy of pathname to caller?
--
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/