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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@xxxxxxxxx>
> Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> ---
>  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@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/