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

[Kasatkin, Dmitry]

On Wed, Feb 27, 2013 at 11:21 AM, Kasatkin, Dmitry
<dmitry.kasatkin@xxxxxxxxx> 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.

Just to clarify... to lock i_mutex before collection (vfs_read),
intead of just before ->setxattr.

>
> 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-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/