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. - Dmitry > thanks, > > Mimi > > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
