ima: prevent dead lock when a file is opened for direct io (take 3)
Subsystems (3)
cover ima: prevent dead lock when a file is opened for direct io (take 3)
Hi,
Files are measured or appraised based on the IMA policy. When a file, in policy, is opened with the O_DIRECT flag, a deadlock occurs. Kernel oops can be seen bellow.
ima_process_measurement() takes the i_mutex, before calculating the hash, appraising the hash against a 'good' known value stored as an extended attribute, and caching the result, before releasing the i_mutex. Holding the i_mutex results in a lockdep with do_blockdev_direct_IO(), which takes the i_mutex and releases it for each read.
The first attempt at resolving this lockdep temporarily removed the O_DIRECT flag and restored it, after calculating the hash. This resolved the lockdep, but the benefits of using O_DIRECT, in terms of page cache, were lost. Al Viro objected to this patch, because it made the "existing locking rule more convoluted". (The existing (undocumented) IMA locking rule prohibits ->read() of regular files from taking the i_mutex. The change would have prohibited ->read() of regular files from taking the i_mutex, unless they were opened with O_DIRECT.)
The second attempt was to introduce the O_DIRECT_HAVELOCK flag for file->f_flags, which would tell to do_blockdev_direct_IO() to skip taking of the i_mutex. This patch did not get any feedback.
This patchset solves O_DIRECT problem with fixing IMA by providing 2 patches.
First patch re-introduce IMA iint->mutex and removes prohibition of ->read of regular files from taking the i_mutex.
As a problem of i_mutex locking is solved, there is now possibility for IMA to use direct-io functionality without temporarily removing O_DIRECT. But that has uncovered that it is impossible to pass kmalloc or vmalloc allocated buffer to the direct-io code, because it writes directly to the user-space pages. In order to overcome this problem, second patch introduces usage of vm_mmap() to allocate user-space like memory like many drivers do.
Thanks,
Dmitry
-----------------------------------------------------------------------
[ 38.221614] =============================================
[ 38.222422] [ INFO: possible recursive locking detected ]
[ 38.223338] 3.13.0-rc7-kds+ #2124 Not tainted
[ 38.223993] ---------------------------------------------
[ 38.224027] openclose-direc/2637 is trying to acquire lock:
[ 38.224027] (&sb->s_type->i_mutex_key#11){+.+.+.}, at: [<ffffffff8116a414>] __blockdev_direct_IO+0x21e/0x2fbd
[ 38.224027]
[ 38.224027] but task is already holding lock:
[ 38.224027] (&sb->s_type->i_mutex_key#11){+.+.+.}, at: [<ffffffff81290748>] process_measurement+0xb0/0x206
[ 38.224027]
[ 38.224027] other info that might help us debug this:
[ 38.224027] Possible unsafe locking scenario:
[ 38.224027]
[ 38.224027] CPU0
[ 38.224027] ----
[ 38.224027] lock(&sb->s_type->i_mutex_key#11);
[ 38.224027] lock(&sb->s_type->i_mutex_key#11);
[ 38.224027]
[ 38.224027] *** DEADLOCK ***
[ 38.224027]
[ 38.224027] May be due to missing lock nesting notation
[ 38.224027]
[ 38.224027] 1 lock held by openclose-direc/2637:
[ 38.224027] #0: (&sb->s_type->i_mutex_key#11){+.+.+.}, at: [<ffffffff81290748>] process_measurement+0xb0/0x206
[ 38.224027]
[ 38.224027] stack backtrace:
[ 38.224027] CPU: 1 PID: 2637 Comm: openclose-direc Not tainted 3.13.0-rc7-kds+ #2124
[ 38.224027] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 38.224027] 0000000000000000 ffff8800363cd4c0 ffffffff814edf5e ffffffff821d7930
[ 38.224027] ffff8800363cd580 ffffffff8107f31e ffffffff8102eb8d ffff8800363cd4e8
[ 38.224027] 0000000081008925 ffff880000000000 ffffffff00000000 0000000000000000
[ 38.224027] Call Trace:
[ 38.224027] [<ffffffff814edf5e>] dump_stack+0x45/0x56
[ 38.224027] [<ffffffff8107f31e>] __lock_acquire+0x802/0x1984
[ 38.224027] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.224027] [<ffffffff8106bf44>] ? sched_clock_cpu+0xc1/0xcc
[ 38.224027] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.224027] [<ffffffff81080a14>] lock_acquire+0xac/0x118
[ 38.224027] [<ffffffff8116a414>] ? __blockdev_direct_IO+0x21e/0x2fbd
[ 38.224027] [<ffffffff814f1a3b>] mutex_lock_nested+0x5e/0x354
[ 38.224027] [<ffffffff8116a414>] ? __blockdev_direct_IO+0x21e/0x2fbd
[ 38.224027] [<ffffffff8116a414>] ? __blockdev_direct_IO+0x21e/0x2fbd
[ 38.224027] [<ffffffff81131c0a>] ? kmem_cache_alloc+0x36/0xf8
[ 38.224027] [<ffffffff8116a3bc>] ? __blockdev_direct_IO+0x1c6/0x2fbd
[ 38.224027] [<ffffffff8116a414>] __blockdev_direct_IO+0x21e/0x2fbd
[ 38.224027] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.224027] [<ffffffff81008925>] ? sched_clock+0x9/0xd
[ 38.224027] [<ffffffff8106bdbc>] ? sched_clock_local+0x12/0x72
[ 38.224027] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.224027] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.224027] [<ffffffff8106bdbc>] ? sched_clock_local+0x12/0x72
[ 38.224027] [<ffffffff8106bf44>] ? sched_clock_cpu+0xc1/0xcc
[ 38.224027] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.224027] [<ffffffff81008925>] ? sched_clock+0x9/0xd
[ 38.224027] [<ffffffff8106bf44>] ? sched_clock_cpu+0xc1/0xcc
[ 38.224027] [<ffffffff811d53b2>] ext4_ind_direct_IO+0x1fb/0x36a
[ 38.224027] [<ffffffff811a525b>] ? _ext4_get_block+0x160/0x160
[ 38.224027] [<ffffffff811a1f1e>] ext4_direct_IO+0x318/0x3c1
[ 38.224027] [<ffffffff811637cd>] ? __find_get_block+0x1a7/0x1e1
[ 38.224027] [<ffffffff810fe46d>] generic_file_aio_read+0xde/0x61a
[ 38.224027] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.224027] [<ffffffff8106bf44>] ? sched_clock_cpu+0xc1/0xcc
[ 38.224027] [<ffffffff81134c56>] do_sync_read+0x59/0x78
[ 38.224027] [<ffffffff81290c75>] ima_kernel_read+0x5f/0x88
[ 38.224027] [<ffffffff81291483>] ima_calc_file_hash+0x1bd/0x229
[ 38.224027] [<ffffffff811573ac>] ? generic_getxattr+0x4f/0x61
[ 38.224027] [<ffffffff81291383>] ? ima_calc_file_hash+0xbd/0x229
[ 38.224027] [<ffffffff812918b2>] ima_collect_measurement+0x8b/0x11e
[ 38.224027] [<ffffffff814f5163>] ? _raw_write_unlock+0x27/0x2a
[ 38.224027] [<ffffffff812907d3>] process_measurement+0x13b/0x206
[ 38.224027] [<ffffffff812909b1>] ima_file_check+0x113/0x11f
[ 38.280032] [<ffffffff8114231b>] do_last+0x937/0xb83
[ 38.280032] [<ffffffff811427ac>] path_openat+0x245/0x633
[ 38.280032] [<ffffffff8102eb8d>] ? kvm_clock_read+0x1f/0x21
[ 38.280032] [<ffffffff81008925>] ? sched_clock+0x9/0xd
[ 38.280032] [<ffffffff81143972>] do_filp_open+0x3a/0x7f
[ 38.280032] [<ffffffff814f4bf4>] ? _raw_spin_unlock+0x27/0x2a
[ 38.280032] [<ffffffff81150a41>] ? __alloc_fd+0x1b7/0x1c9
[ 38.280032] [<ffffffff811348de>] do_sys_open+0x14d/0x1dc
[ 38.280032] [<ffffffff8113498b>] SyS_open+0x1e/0x20
[ 38.280032] [<ffffffff814fcf69>] system_call_fastpath+0x16/0x1b-----------------------------------------------------------------------
Dmitry Kasatkin (2):
ima: re-introduce own integrity cache lock
ima: allocate user-space like memory for direct-io
security/integrity/iint.c | 10 ++++++-- security/integrity/ima/ima_appraise.c | 22 +++++++----------- security/integrity/ima/ima_crypto.c | 44 ++++++++++++++++++++++++++++------- security/integrity/ima/ima_main.c | 27 ++++++++++++--------- security/integrity/integrity.h | 5 ++++ 5 files changed, 73 insertions(+), 35 deletions(-)
Signature · 1 line
1.9.1
PATCH 1/2 ima: re-introduce own integrity cache lock
Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated.
Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex.
To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_mutex.
--- security/integrity/iint.c | 10 ++++++++-- security/integrity/ima/ima_appraise.c | 22 ++++++++-------------- security/integrity/ima/ima_main.c | 27 ++++++++++++++++----------- security/integrity/integrity.h | 5 +++++ 4 files changed, 37 insertions(+), 27 deletions(-)
security/integrity/iint.c +8 -2
@@ -95,13 +95,15 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)struct rb_node *node, *parent =NULL ;struct integrity_iint_cache *iint, *test_iint; +mutex_lock (&inode->i_mutex); + iint =integrity_iint_find (inode);if (iint) -return iint; + goto out; iint =kmem_cache_alloc (iint_cache,GFP_NOFS );if (!iint) -return NULL ; + goto out;write_lock (&integrity_iint_lock);
@@ -123,6 +125,8 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)rb_insert_color (node, &integrity_iint_tree);write_unlock (&integrity_iint_lock); +out: +mutex_unlock (&inode->i_mutex);return iint; }
@@ -154,11 +158,13 @@ static void init_once(void *foo)memset (iint,0 ,sizeof (*iint)); iint->version =0 ; iint->flags =0UL ; + iint->attr_flags =0 ; iint->ima_file_status =INTEGRITY_UNKNOWN ; iint->ima_mmap_status =INTEGRITY_UNKNOWN ; iint->ima_bprm_status =INTEGRITY_UNKNOWN ; iint->ima_module_status =INTEGRITY_UNKNOWN ; iint->evm_status =INTEGRITY_UNKNOWN ; +mutex_init (&iint->mutex); }static int __init integrity_iintcache_init (void )
security/integrity/ima/ima_appraise.c +8 -14
@@ -57,10 +57,12 @@ static int ima_fix_xattr(struct dentry *dentry, iint->ima_hash->xattr.ng.type =IMA_XATTR_DIGEST_NG ; iint->ima_hash->xattr.ng.algo = algo; } +mutex_lock (&dentry->d_inode->i_mutex); rc =__vfs_setxattr_noperm (dentry,XATTR_NAME_IMA , &iint->ima_hash->xattr.data[offset], (sizeof (iint->ima_hash->xattr) - offset) + iint->ima_hash->length,0 ); +mutex_unlock (&dentry->d_inode->i_mutex);return rc; }
@@ -199,7 +201,9 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint, goto out; } +mutex_lock (&inode->i_mutex); status =evm_verifyxattr (dentry,XATTR_NAME_IMA , xattr_value, rc, iint); +mutex_unlock (&inode->i_mutex);if ((status !=INTEGRITY_PASS ) && (status !=INTEGRITY_UNKNOWN )) {if ((status ==INTEGRITY_NOLABEL ) || (status ==INTEGRITY_NOXATTRS ))
@@ -313,13 +317,8 @@ void ima_inode_post_setattr(struct dentry *dentry) must_appraise =ima_must_appraise (inode,MAY_ACCESS ,POST_SETATTR ); iint =integrity_iint_find (inode); -if (iint) { - iint->flags &= ~(IMA_APPRAISE |IMA_APPRAISED | -IMA_APPRAISE_SUBMASK |IMA_APPRAISED_SUBMASK | -IMA_ACTION_FLAGS ); -if (must_appraise) - iint->flags |=IMA_APPRAISE ; - } +if (iint) +set_bit (IMA_CHANGE_ATTR , &iint->attr_flags);if (!must_appraise) rc = inode->i_op->removexattr (dentry,XATTR_NAME_IMA );return ;
@@ -349,13 +348,8 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)return ; iint =integrity_iint_find (inode); -if (!iint) -return ; - - iint->flags &= ~IMA_DONE_MASK ; -if (digsig) - iint->flags |=IMA_DIGSIG ; -return ; +if (iint) +set_bit (IMA_CHANGE_XATTR , &iint->attr_flags); } int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
security/integrity/ima/ima_main.c +16 -11
@@ -88,8 +88,6 @@ static void ima_rdwr_violation_check(struct file *file)if (!S_ISREG (inode->i_mode) || !ima_initialized)return ; -mutex_lock (&inode->i_mutex);/* file metadata: permissions, xattr */ -if (mode &FMODE_WRITE ) {if (atomic_read (&inode->i_readcount) &&IS_IMA (inode)) {struct integrity_iint_cache *iint;
@@ -104,8 +102,6 @@ static void ima_rdwr_violation_check(struct file *file) send_writers = true; } -mutex_unlock (&inode->i_mutex); -if (!send_tomtou && !send_writers)return ;
@@ -127,14 +123,14 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,if (!(mode &FMODE_WRITE ))return ; -mutex_lock (&inode->i_mutex); +mutex_lock (&iint->mutex);if (atomic_read (&inode->i_writecount) ==1 && iint->version != inode->i_version) { iint->flags &= ~IMA_DONE_MASK ;if (iint->flags &IMA_APPRAISE )ima_update_xattr (iint, file); } -mutex_unlock (&inode->i_mutex); +mutex_unlock (&iint->mutex); }/**
@@ -186,11 +182,20 @@ static int process_measurement(struct file *file, const char *filename,/* Is the appraise rule hook specific? */ _func = (action &IMA_FILE_APPRAISE ) ?FILE_CHECK : function; -mutex_lock (&inode->i_mutex); - iint =integrity_inode_get (inode);if (!iint) - goto out; + goto out_unlocked; + +mutex_lock (&iint->mutex); + +if (test_and_clear_bit (IMA_CHANGE_ATTR , &iint->attr_flags)) +/* reset appraisal flags if ima_inode_post_setattr was called */ + iint->flags &= ~(IMA_APPRAISE |IMA_APPRAISED | +IMA_APPRAISE_SUBMASK |IMA_APPRAISED_SUBMASK ); + +if (test_and_clear_bit (IMA_CHANGE_XATTR , &iint->attr_flags)) +/* reset all flags if ima_inode_setxattr was called */ + iint->flags &= ~IMA_DONE_MASK ;/* Determine if already appraised/measured based on bitmask * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
@@ -234,9 +239,9 @@ static int process_measurement(struct file *file, const char *filename, out_digsig:if ((mask &MAY_WRITE ) && (iint->flags &IMA_DIGSIG )) rc = -EACCES ; -out: -mutex_unlock (&inode->i_mutex); +mutex_unlock (&iint->mutex);kfree (xattr_value); +out_unlocked:if ((rc && must_appraise) && (ima_appraise &IMA_APPRAISE_ENFORCE ))return -EACCES ;return 0 ;
security/integrity/integrity.h +5 -0
@@ -50,6 +50,9 @@#define IMA_APPRAISED_SUBMASK (IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \ IMA_BPRM_APPRAISED | IMA_MODULE_APPRAISED) +#define IMA_CHANGE_XATTR 0 +#define IMA_CHANGE_ATTR 1 +enum evm_ima_xattr_type {IMA_XATTR_DIGEST =0x01 ,EVM_XATTR_HMAC ,
@@ -96,9 +99,11 @@ struct signature_v2_hdr {/* integrity data associated with an inode */ struct integrity_iint_cache {struct rb_node rb_node;/* rooted in integrity_iint_tree */ +struct mutex mutex;/* protects: version, flags, digest */ struct inode *inode;/* back pointer to inode in question */ u64 version;/* track inode changes */ unsigned long flags; +unsigned long attr_flags;enum integrity_status ima_file_status:4 ;enum integrity_status ima_mmap_status:4 ;enum integrity_status ima_bprm_status:4 ;
Signature · 1 line
1.9.1
PATCH 2/2 ima: allocate user-space like memory for direct-io
In reply to Dmitry KasatkinFor files opened with O_DIRECT flag, file I/O is done directly to/from user-space pages. kmalloc or vmalloc allocated memory is not suitable for direct IO and set_fs(get_ds()) trick does not work there.
This patch uses vm_mmap() call to allocated annonymous user-space like memory from the kernel.
--- security/integrity/ima/ima_crypto.c | 44 ++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 8 deletions(-)
security/integrity/ima/ima_crypto.c +36 -8
@@ -21,6 +21,7 @@#include <linux/scatterlist.h> #include <linux/err.h> #include <linux/slab.h> +#include <linux/mman.h> #include <crypto/hash.h> #include <crypto/hash_info.h> #include "ima.h"
@@ -36,7 +37,7 @@ static struct crypto_shash *ima_shash_tfm;* */ static int ima_kernel_read (struct file *file,loff_t offset, -char *addr,unsigned long count) +char *addr,unsigned long count,bool kmem) {mm_segment_t old_fs;char __user *buf = addr;
@@ -47,13 +48,16 @@ static int ima_kernel_read(struct file *file, loff_t offset,if (!file->f_op->read && !file->f_op->aio_read)return -EINVAL ; - old_fs =get_fs (); -set_fs (get_ds ()); +if (kmem) { + old_fs =get_fs (); +set_fs (get_ds ()); + }if (file->f_op->read) ret = file->f_op->read (file, buf, count, &offset);else ret =do_sync_read (file, buf, count, &offset); -set_fs (old_fs); +if (kmem) +set_fs (old_fs);return ret; }
@@ -102,6 +106,7 @@ static int ima_calc_file_hash_tfm(struct file *file, {loff_t i_size, offset =0 ;char *rbuf; +unsigned long addr =0 ;int rc, read =0 ;struct {struct shash_desc shash;
@@ -122,9 +127,22 @@ static int ima_calc_file_hash_tfm(struct file *file,if (i_size ==0 ) goto out; - rbuf =kzalloc (PAGE_SIZE ,GFP_KERNEL ); -if (!rbuf) -return -ENOMEM ; +if (file->f_flags &O_DIRECT ) { +/* direct-io code writes to user-space buffers +* set_fs(get_ds()) trick does not work there +*/ + addr =vm_mmap (NULL ,0 ,PAGE_SIZE , +PROT_READ |PROT_WRITE , +MAP_PRIVATE |MAP_ANONYMOUS ,0 ); +if (IS_ERR_VALUE (addr)) +return addr; + } + + rbuf =kmalloc (PAGE_SIZE ,GFP_KERNEL ); +if (!rbuf) { + rc = -ENOMEM ; + goto nomem; + }if (!(file->f_mode &FMODE_READ )) { file->f_mode |=FMODE_READ ;
@@ -134,7 +152,9 @@ static int ima_calc_file_hash_tfm(struct file *file,while (offset < i_size) {int rbuf_len; - rbuf_len =ima_kernel_read (file, offset, rbuf,PAGE_SIZE ); + rbuf_len =ima_kernel_read (file, offset, + addr ? (char *)addr : rbuf, +PAGE_SIZE , !addr);if (rbuf_len <0 ) { rc = rbuf_len;break ;
@@ -143,6 +163,11 @@ static int ima_calc_file_hash_tfm(struct file *file,break ; offset += rbuf_len; +if (addr &&__copy_from_user (rbuf, (void __user *)addr, + rbuf_len)) { + rc = -EFAULT ; +break ; + } rc =crypto_shash_update (&desc.shash, rbuf, rbuf_len);if (rc)break ;
@@ -150,6 +175,9 @@ static int ima_calc_file_hash_tfm(struct file *file,if (read) file->f_mode &= ~FMODE_READ ;kfree (rbuf); +nomem: +if (addr) +vm_munmap (addr,PAGE_SIZE ); out:if (!rc) rc =crypto_shash_final (&desc.shash, hash->digest);
Signature · 1 line
1.9.1