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

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

[Kasatkin, Dmitry]

On Wed, Feb 20, 2013 at 11:27 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Hi Al,
>
> Are there any negative repercussions to temporarily removing the
> o_direct flag in order to calculate the file hash?
>

It looks to me that there should not be any problem to
setting/unsetting O_DIRECT flag.
This behavior is already supported for user space by the kernel using:
fcntl(fd, F_SETFL, ...)
From manual page:
              On  Linux this command can change only the O_APPEND, O_ASYNC,
              O_DIRECT, O_NOATIME, and O_NONBLOCK flags.

In kernel, it calls setfl(), which may just set or unset O_DIRECT flag.

It seems that unsetting/setting O_DIRECT is also well possible for
kernel_read().

- Dmitry

> thanks,
>
> Mimi
> -----
>
> Files are measured or appraised based on the IMA policy.  When a file
> in policy is opened for read with the O_DIRECT flag set, a deadlock
> occurs due to do_blockdev_direct_IO() taking i_mutex before calling
> filemap_write_and_wait_range(). The i_mutex was previously taken in
> process_measurement().  This patch temporarily removes the O_DIRECT
> flag in order to calculate the hash and restores it once completed.
>
> [    3.751980]
> [    3.752074] =============================================
> [    3.752074] [ INFO: possible recursive locking detected ]
> [    3.752074] 3.7.5+ #30 Not tainted
> [    3.752074] ---------------------------------------------
> [    3.752074] startpar/1067 is trying to acquire lock:
> [    3.752074]  (&sb->s_type->i_mutex_key#10){+.+.+.}, at: [<c1156866>] do_blockdev_direct_IO+0x16a6/0x1900
> [    3.752074]
> [    3.752074] but task is already holding lock:
> [    3.752074]  (&sb->s_type->i_mutex_key#10){+.+.+.}, at: [<c125cbd1>] process_measurement+0x71/0x1f0
> [    3.752074]
> [    3.752074] other info that might help us debug this:
> [    3.752074]  Possible unsafe locking scenario:
> [    3.752074]
> [    3.752074]        CPU0
> [    3.752074]        ----
> [    3.752074]   lock(&sb->s_type->i_mutex_key#10);
> [    3.752074]   lock(&sb->s_type->i_mutex_key#10);
> [    3.752074]
> [    3.752074]  *** DEADLOCK ***
> [    3.752074]
> [    3.752074]  May be due to missing lock nesting notation
> [    3.752074]
> [    3.752074] 1 lock held by startpar/1067:
> [    3.752074]  #0:  (&sb->s_type->i_mutex_key#10){+.+.+.}, at: [<c125cbd1>] process_measurement+0x71/0x1f0
> [    3.752074]
> [    3.752074] stack backtrace:
> [    3.752074] Pid: 1067, comm: startpar Not tainted 3.7.5+ #30
> [    3.752074] Call Trace:
> [    3.752074]  [<c108b57a>] __lock_acquire+0x5da/0x1490
> [    3.752074]  [<c108ad6b>] ? trace_hardirqs_on+0xb/0x10
> [    3.752074]  [<c108c4b2>] lock_acquire+0x82/0xf0
> [    3.752074]  [<c1156866>] ? do_blockdev_direct_IO+0x16a6/0x1900
> [    3.752074]  [<c16e9256>] __mutex_lock_common+0x46/0x350
> [    3.752074]  [<c1156866>] ? do_blockdev_direct_IO+0x16a6/0x1900
> [    3.752074]  [<c111a99d>] ? kmem_cache_alloc+0xcd/0x120
> [    3.752074]  [<c16e9651>] mutex_lock_nested+0x31/0x40
> [    3.752074]  [<c1156866>] ? do_blockdev_direct_IO+0x16a6/0x1900
> [    3.752074]  [<c1156866>] do_blockdev_direct_IO+0x16a6/0x1900
> [    3.752074]  [<c106f7d6>] ? find_busiest_group+0x26/0x440
> [    3.752074]  [<c10652b8>] ? finish_task_switch+0x38/0xe0
> [    3.752074]  [<c1156b30>] __blockdev_direct_IO+0x70/0x80
> [    3.752074]  [<c118ed90>] ? noalloc_get_block_write+0x50/0x50
> [    3.752074]  [<c11cb3d0>] ext4_ind_direct_IO+0x120/0x500
> [    3.752074]  [<c118ed90>] ? noalloc_get_block_write+0x50/0x50
> [    3.752074]  [<c1190426>] ext4_direct_IO+0x276/0x430
> [    3.752074]  [<c10e2597>] generic_file_aio_read+0x6f7/0x740
> [    3.752074]  [<c108b2b0>] ? __lock_acquire+0x310/0x1490
> [    3.752074]  [<c1086920>] ? noop_count+0x10/0x10
> [    3.752074]  [<c111f653>] do_sync_read+0x93/0xd0
> [    3.752074]  [<c111f997>] vfs_read+0x97/0x160
> [    3.752074]  [<c111f5c0>] ? do_sync_write+0xd0/0xd0
> [    3.752074]  [<c11255f0>] kernel_read+0x30/0x50
> [    3.752074]  [<c125d23b>] ima_calc_hash+0xbb/0x1c0
> [    3.752074]  [<c1060476>] ? lg_local_unlock+0x16/0x30
> [    3.752074]  [<c113cb27>] ? mntput_no_expire+0x37/0x120
> [    3.752074]  [<c125d466>] ima_collect_measurement+0x56/0xa0
> [    3.752074]  [<c1133cb2>] ? d_path+0xb2/0xd0
> [    3.752074]  [<c125cca6>] process_measurement+0x146/0x1f0
> [    3.752074]  [<c125cd93>] ima_file_check+0x43/0x1c0
> [    3.752074]  [<c112ae95>] do_last+0x485/0xb80
> [    3.752074]  [<c1128e81>] ? inode_permission+0x11/0x50
> [    3.752074]  [<c112b5f8>] ? link_path_walk+0x68/0x760
> [    3.752074]  [<c112d7fd>] path_openat+0x9d/0x3a0
> [    3.752074]  [<c12ff96c>] ? tty_release+0x32c/0x4c0
> [    3.752074]  [<c112dbf0>] do_filp_open+0x30/0x80
> [    3.752074]  [<c111dc9f>] do_sys_open+0xef/0x1d0
> [    3.752074]  [<c111dded>] sys_open+0x2d/0x40
> [    3.752074]  [<c16ebe4c>] syscall_call+0x7/0xb
>
> Reported-by: Cédric BERTHION <cedric.berthion@amossys.fr>
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima_crypto.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index b691e0f..99aea6a 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -45,6 +45,7 @@ int ima_calc_file_hash(struct file *file, char *digest)
>         loff_t i_size, offset = 0;
>         char *rbuf;
>         int rc, read = 0;
> +       unsigned int unset_flags = file->f_flags & O_DIRECT;
>         struct {
>                 struct shash_desc shash;
>                 char ctx[crypto_shash_descsize(ima_shash_tfm)];
> @@ -62,6 +63,10 @@ int ima_calc_file_hash(struct file *file, char *digest)
>                 rc = -ENOMEM;
>                 goto out;
>         }
> +
> +       if (unset_flags)
> +               file->f_flags &= ~unset_flags;
> +
>         if (!(file->f_mode & FMODE_READ)) {
>                 file->f_mode |= FMODE_READ;
>                 read = 1;
> @@ -88,6 +93,8 @@ int ima_calc_file_hash(struct file *file, char *digest)
>                 rc = crypto_shash_final(&desc.shash, digest);
>         if (read)
>                 file->f_mode &= ~FMODE_READ;
> +       if (unset_flags)
> +               file->f_flags |= unset_flags;
>  out:
>         return rc;
>  }
> --
> 1.8.1.rc3
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kasatkin, Dmitry]

On Wed, Feb 27, 2013 at 1:22 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@intel.com> wrote:
> On Wed, Feb 27, 2013 at 1:22 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mimi Zohar]

On Wed, 2013-02-27 at 14:26 +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 27, 2013 at 11:21 AM, Kasatkin, Dmitry
> <dmitry.kasatkin@intel.com> wrote:
> > On Wed, Feb 27, 2013 at 1:22 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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.

Right, when writing 'security.ima' on __fput(), where it is now
permissible to take the i_mutex, we need to ensure that the measured
content of the file and writing the xattr are in sync.

Here, in process_measurement(), the purpose of taking the i_mutex before
calculating the file hash, as opposed to before reading xattrs, is to
tie together calculating the hash, reading the xattrs used in verifying
file integrity, and updating the iint status, so the hash calculation is
done only once.

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

Specifically, chmod took the i_mutex and then iint->mutex, while
ima_file_free() and process_measurement() took the locks in reverse
order.

thanks,

Mimi

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

[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@linux.vnet.ibm.com> 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?

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

[Mimi Zohar]

On Wed, 2013-02-27 at 19:00 +0000, Al Viro wrote:
> 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@linux.vnet.ibm.com> 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?

The name helps userspace correlate a file hash with a specific file:

- cat /sys/kernel/security/ima/ascii_runtime_measurements

The measurement list is being used for attestation.

LSS 2012: The Linux Integrity Measurement Architecture and
TPM-Based Network Endpoint Assessment - Andreas Steffen
http://kernsec.org/files/LSS_2012_strongSwan_IMA.pdf

- "audit log hashes" is being used, I assume, for big data analytics
- auditing integrity errors

thanks,

Mimi