* [PATCH 1/2] ima: re-introduce own integrity cache lock
2014-05-13 15:59 [PATCH 0/2] ima: prevent dead lock when a file is opened for direct io (take 3) Dmitry Kasatkin
@ 2014-05-13 15:59 ` Dmitry Kasatkin
2014-05-13 15:59 ` [PATCH 2/2] ima: allocate user-space like memory for direct-io Dmitry Kasatkin
1 sibling, 0 replies; 3+ messages in thread
From: Dmitry Kasatkin @ 2014-05-13 15:59 UTC (permalink / raw)
To: viro, linux-security-module, eparis, zohar, linux-ima-devel
Cc: linux-kernel, hooanon05g, jmorris, dmitry.kasatkin,
Dmitry Kasatkin
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.
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
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(-)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index a521edf..4c202b7 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -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)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index d3113d4..0f8579e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -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,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index bd7b4cb..1e63c91 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -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;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 2fb5e53..f73cd06 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -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;
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] ima: allocate user-space like memory for direct-io
2014-05-13 15:59 [PATCH 0/2] ima: prevent dead lock when a file is opened for direct io (take 3) Dmitry Kasatkin
2014-05-13 15:59 ` [PATCH 1/2] ima: re-introduce own integrity cache lock Dmitry Kasatkin
@ 2014-05-13 15:59 ` Dmitry Kasatkin
1 sibling, 0 replies; 3+ messages in thread
From: Dmitry Kasatkin @ 2014-05-13 15:59 UTC (permalink / raw)
To: viro, linux-security-module, eparis, zohar, linux-ima-devel
Cc: linux-kernel, hooanon05g, jmorris, dmitry.kasatkin,
Dmitry Kasatkin
For 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.
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
security/integrity/ima/ima_crypto.c | 44 ++++++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 8 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index de5b974..59efc0a 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -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);
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread