* [PATCH v3 01/12] vfs: extend vfs_removexattr locking
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 02/12] vfs: move ima_file_free before releasing the file Mimi Zohar
` (10 subsequent siblings)
11 siblings, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Mimi Zohar, Dmitry Kasatkin
This patch takes the i_mutex lock before security_inode_removexattr(),
instead of after, in preparation of calling ima_inode_removexattr().
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@nokia.com>
---
fs/xattr.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/xattr.c b/fs/xattr.c
index 82f4337..6112c92 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -294,11 +294,13 @@ vfs_removexattr(struct dentry *dentry, const char *name)
if (error)
return error;
+ mutex_lock(&inode->i_mutex);
error = security_inode_removexattr(dentry, name);
- if (error)
+ if (error) {
+ mutex_unlock(&inode->i_mutex);
return error;
+ }
- mutex_lock(&inode->i_mutex);
error = inode->i_op->removexattr(dentry, name);
mutex_unlock(&inode->i_mutex);
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 02/12] vfs: move ima_file_free before releasing the file
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 01/12] vfs: extend vfs_removexattr locking Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-22 14:23 ` Kasatkin, Dmitry
2012-03-21 18:54 ` [PATCH v3 03/12] ima: free securityfs violations file Mimi Zohar
` (9 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Mimi Zohar
ima_file_free(), called on __fput(), currently flags files that have
changed, so that the file is re-measured. For appraising a files's
integrity, the file's hash must be re-calculated and stored in the
'security.ima' xattr to reflect any changes.
This patch moves the ima_file_free() call to before releasing the file
in preparation of ima-appraisal measuring the file and updating the
'security.ima' xattr.
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
fs/file_table.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index 20002e3..554161a 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -243,10 +243,10 @@ static void __fput(struct file *file)
if (file->f_op && file->f_op->fasync)
file->f_op->fasync(-1, file, 0);
}
+ ima_file_free(file);
if (file->f_op && file->f_op->release)
file->f_op->release(inode, file);
security_file_free(file);
- ima_file_free(file);
if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
!(file->f_mode & FMODE_PATH))) {
cdev_put(inode->i_cdev);
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 02/12] vfs: move ima_file_free before releasing the file
2012-03-21 18:54 ` [PATCH v3 02/12] vfs: move ima_file_free before releasing the file Mimi Zohar
@ 2012-03-22 14:23 ` Kasatkin, Dmitry
0 siblings, 0 replies; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 14:23 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
On Wed, Mar 21, 2012 at 8:54 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> ima_file_free(), called on __fput(), currently flags files that have
> changed, so that the file is re-measured. For appraising a files's
> integrity, the file's hash must be re-calculated and stored in the
> 'security.ima' xattr to reflect any changes.
>
> This patch moves the ima_file_free() call to before releasing the file
> in preparation of ima-appraisal measuring the file and updating the
> 'security.ima' xattr.
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> Acked-by: Serge Hallyn <serge.hallyn@ubuntu.com>
Acked-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> ---
> fs/file_table.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 20002e3..554161a 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -243,10 +243,10 @@ static void __fput(struct file *file)
> if (file->f_op && file->f_op->fasync)
> file->f_op->fasync(-1, file, 0);
> }
> + ima_file_free(file);
> if (file->f_op && file->f_op->release)
> file->f_op->release(inode, file);
> security_file_free(file);
> - ima_file_free(file);
> if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
> !(file->f_mode & FMODE_PATH))) {
> cdev_put(inode->i_cdev);
> --
> 1.7.6.5
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 03/12] ima: free securityfs violations file
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 01/12] vfs: extend vfs_removexattr locking Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 02/12] vfs: move ima_file_free before releasing the file Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 04/12] ima: integrity appraisal extension Mimi Zohar
` (8 subsequent siblings)
11 siblings, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Dmitry Kasatkin, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
From: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
On ima_fs_init() error, free securityfs violations file.
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
security/integrity/ima/ima_fs.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e1aa2b4..3fccc06 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -367,6 +367,7 @@ int __init ima_fs_init(void)
return 0;
out:
+ securityfs_remove(violations);
securityfs_remove(runtime_measurements_count);
securityfs_remove(ascii_runtime_measurements);
securityfs_remove(binary_runtime_measurements);
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 04/12] ima: integrity appraisal extension
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (2 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 03/12] ima: free securityfs violations file Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-22 14:28 ` Kasatkin, Dmitry
2012-03-21 18:54 ` [PATCH v3 05/12] ima: add appraise action keywords and default rules Mimi Zohar
` (7 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Mimi Zohar
IMA currently maintains an integrity measurement list used to assert the
integrity of the running system to a third party. The IMA-appraisal
extension adds local integrity validation and enforcement of the
measurement against a "good" value stored as an extended attribute
'security.ima'. The initial methods for validating 'security.ima' are
hashed based, which provides file data integrity, and digital signature
based, which in addition to providing file data integrity, provides
authenticity.
This patch creates and maintains the 'security.ima' xattr, containing
the file data hash measurement. Protection of the xattr is provided by
EVM, if enabled and configured.
Based on policy, IMA calls evm_verifyxattr() to verify a file's metadata
integrity and, assuming success, compares the file's current hash value
with the one stored as an extended attribute in 'security.ima'.
Changelog v3:
- change appraisal default for filesystems without xattr support to fail
Changelog v2:
- fix audit msg 'res' value
- removed unused 'ima_appraise=' values
Changelog v1:
- removed unused iint mutex (Dmitry Kasatkin)
- setattr hook must not reset appraised (Dmitry Kasatkin)
- evm_verifyxattr() now differentiates between no 'security.evm' xattr
(INTEGRITY_NOLABEL) and no EVM 'protected' xattrs included in the
'security.evm' (INTEGRITY_NOXATTRS).
- replace hash_status with ima_status (Dmitry Kasatkin)
- re-initialize slab element ima_status on free (Dmitry Kasatkin)
- include 'security.ima' in EVM if CONFIG_IMA_APPRAISE, not CONFIG_IMA
- merged half "ima: ima_must_appraise_or_measure API change" (Dmitry Kasatkin)
- removed unnecessary error variable in process_measurement() (Dmitry Kasatkin)
- use ima_inode_post_setattr() stub function, if IMA_APPRAISE not configured
(moved ima_inode_post_setattr() to ima_appraise.c)
- make sure ima_collect_measurement() can read file
Changelog:
- add 'iint' to evm_verifyxattr() call (Dimitry Kasatkin)
- fix the race condition between chmod, which takes the i_mutex and then
iint->mutex, and ima_file_free() and process_measurement(), which take
the locks in the reverse order, by eliminating iint->mutex. (Dmitry Kasatkin)
- cleanup of ima_appraise_measurement() (Dmitry Kasatkin)
- changes as a result of the iint not allocated for all regular files, but
only for those measured/appraised.
- don't try to appraise new/empty files
- expanded ima_appraisal description in ima/Kconfig
- IMA appraise definitions required even if IMA_APPRAISE not enabled
- add return value to ima_must_appraise() stub
- unconditionally set status = INTEGRITY_PASS *after* testing status,
not before. (Found by Joe Perches)
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
Documentation/kernel-parameters.txt | 4 +
include/linux/xattr.h | 3 +
security/integrity/evm/evm_main.c | 3 +
security/integrity/iint.c | 3 +-
security/integrity/ima/Kconfig | 15 +++
security/integrity/ima/Makefile | 2 +
security/integrity/ima/ima.h | 37 +++++++-
security/integrity/ima/ima_api.c | 50 +++++++---
security/integrity/ima/ima_appraise.c | 168 +++++++++++++++++++++++++++++++++
security/integrity/ima/ima_crypto.c | 8 ++-
security/integrity/ima/ima_main.c | 78 ++++++++++-----
security/integrity/ima/ima_policy.c | 32 +++++--
security/integrity/integrity.h | 8 +-
13 files changed, 358 insertions(+), 53 deletions(-)
create mode 100644 security/integrity/ima/ima_appraise.c
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 033d4e6..a86765d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1004,6 +1004,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
ihash_entries= [KNL]
Set number of hash buckets for inode cache.
+ ima_appraise= [IMA] appraise integrity measurements
+ Format: { "off" | "enforce" | "fix" }
+ default: "enforce"
+
ima_audit= [IMA]
Format: { "0" | "1" }
0 -- integrity auditing messages. (Default)
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index e5d1220..77a3e68 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -33,6 +33,9 @@
#define XATTR_EVM_SUFFIX "evm"
#define XATTR_NAME_EVM XATTR_SECURITY_PREFIX XATTR_EVM_SUFFIX
+#define XATTR_IMA_SUFFIX "ima"
+#define XATTR_NAME_IMA XATTR_SECURITY_PREFIX XATTR_IMA_SUFFIX
+
#define XATTR_SELINUX_SUFFIX "selinux"
#define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 8901501..eb54845 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -34,6 +34,9 @@ char *evm_config_xattrnames[] = {
#ifdef CONFIG_SECURITY_SMACK
XATTR_NAME_SMACK,
#endif
+#ifdef CONFIG_IMA_APPRAISE
+ XATTR_NAME_IMA,
+#endif
XATTR_NAME_CAPS,
NULL
};
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 399641c..e600986 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -74,6 +74,7 @@ static void iint_free(struct integrity_iint_cache *iint)
{
iint->version = 0;
iint->flags = 0UL;
+ iint->ima_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
kmem_cache_free(iint_cache, iint);
}
@@ -157,7 +158,7 @@ static void init_once(void *foo)
memset(iint, 0, sizeof *iint);
iint->version = 0;
iint->flags = 0UL;
- mutex_init(&iint->mutex);
+ iint->ima_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
}
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 35664fe..b7465a1 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -54,3 +54,18 @@ config IMA_LSM_RULES
default y
help
Disabling this option will disregard LSM based policy rules.
+
+config IMA_APPRAISE
+ bool "Appraise integrity measurements"
+ depends on IMA
+ default n
+ help
+ This option enables local measurement integrity appraisal.
+ It requires the system to be labeled with a security extended
+ attribute containing the file hash measurement. To protect
+ the security extended attributes from offline attack, enable
+ and configure EVM.
+
+ For more information on integrity appraisal refer to:
+ <http://linux-ima.sourceforge.net>
+ If unsure, say N.
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 5690c02..bd31516 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -7,3 +7,5 @@ obj-$(CONFIG_IMA) += ima.o
ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
ima_policy.o ima_audit.o
+
+ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3ccf7ac..d5bf463 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -40,6 +40,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
extern int ima_initialized;
extern int ima_used_chip;
extern char *ima_hash;
+extern int ima_appraise;
/* IMA inode template definition */
struct ima_template_data {
@@ -98,6 +99,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
}
/* LIM API function definitions */
+int ima_must_appraise_or_measure(struct inode *inode, int mask, int function);
int ima_must_measure(struct inode *inode, int mask, int function);
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file);
@@ -114,14 +116,45 @@ struct integrity_iint_cache *integrity_iint_insert(struct inode *inode);
struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
/* IMA policy related functions */
-enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
+enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK, POST_SETATTR };
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
+int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
+ int flags);
void ima_init_policy(void);
void ima_update_policy(void);
ssize_t ima_parse_add_rule(char *);
void ima_delete_rules(void);
+/* Appraise integrity measurements */
+#define IMA_APPRAISE_ENFORCE 0x01
+#define IMA_APPRAISE_FIX 0x02
+
+#ifdef CONFIG_IMA_APPRAISE
+int ima_appraise_measurement(struct integrity_iint_cache *iint,
+ struct file *file, const unsigned char *filename);
+int ima_must_appraise(struct inode *inode, enum ima_hooks func, int mask);
+void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
+
+#else
+static inline int ima_appraise_measurement(struct integrity_iint_cache *iint,
+ struct file *file,
+ const unsigned char *filename)
+{
+ return INTEGRITY_UNKNOWN;
+}
+
+static inline int ima_must_appraise(struct inode *inode,
+ enum ima_hooks func, int mask)
+{
+ return 0;
+}
+
+static inline void ima_update_xattr(struct integrity_iint_cache *iint,
+ struct file *file)
+{
+}
+#endif
+
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 88a2788..55deeb1 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -9,13 +9,17 @@
* License.
*
* File: ima_api.c
- * Implements must_measure, collect_measurement, store_measurement,
- * and store_template.
+ * Implements must_appraise_or_measure, collect_measurement,
+ * appraise_measurement, store_measurement and store_template.
*/
#include <linux/module.h>
#include <linux/slab.h>
-
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/xattr.h>
+#include <linux/evm.h>
#include "ima.h"
+
static const char *IMA_TEMPLATE_NAME = "ima";
/*
@@ -93,7 +97,7 @@ err_out:
}
/**
- * ima_must_measure - measure decision based on policy.
+ * ima_must_appraise_or_measure - appraise & measure decision based on policy.
* @inode: pointer to inode to measure
* @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXECUTE)
* @function: calling function (FILE_CHECK, BPRM_CHECK, FILE_MMAP)
@@ -105,15 +109,22 @@ err_out:
* mask: contains the permission mask
* fsmagic: hex value
*
- * Return 0 to measure. For matching a DONT_MEASURE policy, no policy,
- * or other error, return an error code.
-*/
-int ima_must_measure(struct inode *inode, int mask, int function)
+ * Returns IMA_MEASURE, IMA_APPRAISE mask.
+ *
+ */
+int ima_must_appraise_or_measure(struct inode *inode, int mask, int function)
{
- int must_measure;
+ int flags = IMA_MEASURE | IMA_APPRAISE;
+
+ if (!ima_appraise)
+ flags &= ~IMA_APPRAISE;
+
+ return ima_match_policy(inode, function, mask, flags);
+}
- must_measure = ima_match_policy(inode, function, mask);
- return must_measure ? 0 : -EACCES;
+int ima_must_measure(struct inode *inode, int mask, int function)
+{
+ return ima_match_policy(inode, function, mask, IMA_MEASURE);
}
/*
@@ -129,16 +140,24 @@ int ima_must_measure(struct inode *inode, int mask, int function)
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file)
{
- int result = -EEXIST;
+ struct inode *inode = file->f_dentry->d_inode;
+ const char *filename = file->f_dentry->d_name.name;
+ int result = 0;
- if (!(iint->flags & IMA_MEASURED)) {
+ if (!(iint->flags & IMA_COLLECTED)) {
u64 i_version = file->f_dentry->d_inode->i_version;
memset(iint->digest, 0, IMA_DIGEST_SIZE);
result = ima_calc_hash(file, iint->digest);
- if (!result)
+ if (!result) {
iint->version = i_version;
+ iint->flags |= IMA_COLLECTED;
+ }
}
+ if (result)
+ integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
+ filename, "collect_data", "failed",
+ result, 0);
return result;
}
@@ -167,6 +186,9 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
struct ima_template_entry *entry;
int violation = 0;
+ if (iint->flags & IMA_MEASURED)
+ return;
+
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
if (!entry) {
integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
new file mode 100644
index 0000000..4865f61
--- /dev/null
+++ b/security/integrity/ima/ima_appraise.c
@@ -0,0 +1,168 @@
+/*
+ * Copyright (C) 2011 IBM Corporation
+ *
+ * Author:
+ * Mimi Zohar <zohar@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+#include <linux/module.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/xattr.h>
+#include <linux/magic.h>
+#include <linux/ima.h>
+#include <linux/evm.h>
+
+#include "ima.h"
+
+static int __init default_appraise_setup(char *str)
+{
+ if (strncmp(str, "off", 3) == 0)
+ ima_appraise = 0;
+ else if (strncmp(str, "fix", 3) == 0)
+ ima_appraise = IMA_APPRAISE_FIX;
+ return 1;
+}
+
+__setup("ima_appraise=", default_appraise_setup);
+
+/*
+ * ima_must_appraise - set appraise flag
+ *
+ * Return 1 to appraise
+ */
+int ima_must_appraise(struct inode *inode, enum ima_hooks func, int mask)
+{
+ return 0;
+}
+
+static void ima_fix_xattr(struct dentry *dentry,
+ struct integrity_iint_cache *iint)
+{
+ iint->digest[0] = IMA_XATTR_DIGEST;
+ __vfs_setxattr_noperm(dentry, XATTR_NAME_IMA,
+ iint->digest, IMA_DIGEST_SIZE + 1, 0);
+}
+
+/*
+ * ima_appraise_measurement - appraise file measurement
+ *
+ * Call evm_verifyxattr() to verify the integrity of 'security.ima'.
+ * Assuming success, compare the xattr hash with the collected measurement.
+ *
+ * Return 0 on success, error code otherwise
+ */
+int ima_appraise_measurement(struct integrity_iint_cache *iint,
+ struct file *file, const unsigned char *filename)
+{
+ struct dentry *dentry = file->f_dentry;
+ struct inode *inode = dentry->d_inode;
+ u8 xattr_value[IMA_DIGEST_SIZE];
+ enum integrity_status status = INTEGRITY_UNKNOWN;
+ const char *op = "appraise_data";
+ char *cause = "unknown";
+ int rc;
+
+ if (!ima_appraise)
+ return 0;
+ if (!inode->i_op->getxattr)
+ return INTEGRITY_UNKNOWN;
+
+ if (iint->flags & IMA_APPRAISED)
+ return iint->ima_status;
+
+ rc = inode->i_op->getxattr(dentry, XATTR_NAME_IMA, xattr_value,
+ IMA_DIGEST_SIZE);
+ if (rc <= 0) {
+ if (rc && rc != -ENODATA)
+ goto out;
+
+ cause = "missing-hash";
+ status =
+ (inode->i_size == 0) ? INTEGRITY_PASS : INTEGRITY_NOLABEL;
+ goto out;
+ }
+
+ status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
+ if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
+ if ((status == INTEGRITY_NOLABEL)
+ || (status == INTEGRITY_NOXATTRS))
+ cause = "missing-HMAC";
+ else if (status == INTEGRITY_FAIL)
+ cause = "invalid-HMAC";
+ goto out;
+ }
+
+ rc = memcmp(xattr_value, iint->digest, IMA_DIGEST_SIZE);
+ if (rc) {
+ status = INTEGRITY_FAIL;
+ cause = "invalid-hash";
+ print_hex_dump_bytes("security.ima: ", DUMP_PREFIX_NONE,
+ xattr_value, IMA_DIGEST_SIZE);
+ print_hex_dump_bytes("collected: ", DUMP_PREFIX_NONE,
+ iint->digest, IMA_DIGEST_SIZE);
+ goto out;
+ }
+ status = INTEGRITY_PASS;
+ iint->flags |= IMA_APPRAISED;
+out:
+ if (status != INTEGRITY_PASS) {
+ if (ima_appraise & IMA_APPRAISE_FIX) {
+ ima_fix_xattr(dentry, iint);
+ status = INTEGRITY_PASS;
+ }
+ integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
+ op, cause, rc, 0);
+ }
+ iint->ima_status = status;
+ return status;
+}
+
+/*
+ * ima_update_xattr - update 'security.ima' hash value
+ */
+void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
+{
+ struct dentry *dentry = file->f_dentry;
+ int rc = 0;
+
+ rc = ima_collect_measurement(iint, file);
+ if (rc < 0)
+ return;
+ ima_fix_xattr(dentry, iint);
+}
+
+/**
+ * ima_inode_post_setattr - reflect file metadata changes
+ * @dentry: pointer to the affected dentry
+ *
+ * Changes to a dentry's metadata might result in needing to appraise.
+ *
+ * This function is called from notify_change(), which expects the caller
+ * to lock the inode's i_mutex.
+ */
+void ima_inode_post_setattr(struct dentry *dentry)
+{
+ struct inode *inode = dentry->d_inode;
+ struct integrity_iint_cache *iint;
+ int must_appraise, rc;
+
+ if (!ima_initialized || !ima_appraise || !S_ISREG(inode->i_mode)
+ || !inode->i_op->removexattr)
+ return;
+
+ must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
+ iint = integrity_iint_find(inode);
+ if (iint) {
+ if (must_appraise)
+ iint->flags |= IMA_APPRAISE;
+ else
+ iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED);
+ }
+ if (!must_appraise)
+ rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
+ return;
+}
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 9b3ade7..b21ee5b 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -48,7 +48,7 @@ int ima_calc_hash(struct file *file, char *digest)
struct scatterlist sg[1];
loff_t i_size, offset = 0;
char *rbuf;
- int rc;
+ int rc, read = 0;
rc = init_desc(&desc);
if (rc != 0)
@@ -59,6 +59,10 @@ int ima_calc_hash(struct file *file, char *digest)
rc = -ENOMEM;
goto out;
}
+ if (!(file->f_mode & FMODE_READ)) {
+ file->f_mode |= FMODE_READ;
+ read = 1;
+ }
i_size = i_size_read(file->f_dentry->d_inode);
while (offset < i_size) {
int rbuf_len;
@@ -80,6 +84,8 @@ int ima_calc_hash(struct file *file, char *digest)
kfree(rbuf);
if (!rc)
rc = crypto_hash_final(&desc, digest);
+ if (read)
+ file->f_mode &= ~FMODE_READ;
out:
crypto_free_hash(desc.tfm);
return rc;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5b222eb..fba2f7b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -22,12 +22,19 @@
#include <linux/mount.h>
#include <linux/mman.h>
#include <linux/slab.h>
+#include <linux/xattr.h>
#include <linux/ima.h>
#include "ima.h"
int ima_initialized;
+#ifdef CONFIG_IMA_APPRAISE
+int ima_appraise = IMA_APPRAISE_ENFORCE;
+#else
+int ima_appraise;
+#endif
+
char *ima_hash = "sha1";
static int __init hash_setup(char *str)
{
@@ -52,7 +59,7 @@ static void ima_rdwr_violation_check(struct file *file)
struct dentry *dentry = file->f_path.dentry;
struct inode *inode = dentry->d_inode;
fmode_t mode = file->f_mode;
- int rc;
+ int must_measure;
bool send_tomtou = false, send_writers = false;
if (!S_ISREG(inode->i_mode) || !ima_initialized)
@@ -66,8 +73,8 @@ static void ima_rdwr_violation_check(struct file *file)
goto out;
}
- rc = ima_must_measure(inode, MAY_READ, FILE_CHECK);
- if (rc < 0)
+ must_measure = ima_must_measure(inode, MAY_READ, FILE_CHECK);
+ if (!must_measure)
goto out;
if (atomic_read(&inode->i_writecount) > 0)
@@ -84,17 +91,21 @@ out:
}
static void ima_check_last_writer(struct integrity_iint_cache *iint,
- struct inode *inode,
- struct file *file)
+ struct inode *inode, struct file *file)
{
fmode_t mode = file->f_mode;
- mutex_lock(&iint->mutex);
- if (mode & FMODE_WRITE &&
- atomic_read(&inode->i_writecount) == 1 &&
- iint->version != inode->i_version)
- iint->flags &= ~IMA_MEASURED;
- mutex_unlock(&iint->mutex);
+ if (!(mode & FMODE_WRITE))
+ return;
+
+ mutex_lock(&inode->i_mutex);
+ if (atomic_read(&inode->i_writecount) == 1 &&
+ iint->version != inode->i_version) {
+ iint->flags &= ~(IMA_COLLECTED | IMA_APPRAISED | IMA_MEASURED);
+ if (iint->flags & IMA_APPRAISE)
+ ima_update_xattr(iint, file);
+ }
+ mutex_unlock(&inode->i_mutex);
}
/**
@@ -123,14 +134,17 @@ static int process_measurement(struct file *file, const unsigned char *filename,
{
struct inode *inode = file->f_dentry->d_inode;
struct integrity_iint_cache *iint;
- int rc = 0;
+ int rc = -ENOMEM, action, must_appraise;
if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
- rc = ima_must_measure(inode, mask, function);
- if (rc != 0)
- return rc;
+ /* Determine if in appraise/measurement policy,
+ * returns IMA_MEASURE, IMA_APPRAISE bitmask. */
+ action = ima_must_appraise_or_measure(inode, mask, function);
+ if (!action)
+ return 0;
+
retry:
iint = integrity_iint_find(inode);
if (!iint) {
@@ -140,18 +154,32 @@ retry:
return rc;
}
- mutex_lock(&iint->mutex);
+ must_appraise = action & IMA_APPRAISE;
- rc = iint->flags & IMA_MEASURED ? 1 : 0;
- if (rc != 0)
+ mutex_lock(&inode->i_mutex);
+
+ /* Determine if already appraised/measured based on bitmask
+ * (IMA_MEASURE, IMA_MEASURED, IMA_APPRAISE, IMA_APPRAISED) */
+ iint->flags |= action;
+ action &= ~((iint->flags & (IMA_MEASURED | IMA_APPRAISED)) >> 1);
+
+ /* Nothing to do, just return existing appraised status */
+ if (!action) {
+ if (iint->flags & IMA_APPRAISED)
+ rc = iint->ima_status;
goto out;
+ }
rc = ima_collect_measurement(iint, file);
- if (!rc)
+ if (rc != 0)
+ goto out;
+ if (action & IMA_MEASURE)
ima_store_measurement(iint, file, filename);
+ if (action & IMA_APPRAISE)
+ rc = ima_appraise_measurement(iint, file, filename);
out:
- mutex_unlock(&iint->mutex);
- return rc;
+ mutex_unlock(&inode->i_mutex);
+ return (rc && must_appraise) ? -EACCES : 0;
}
/**
@@ -167,14 +195,14 @@ out:
*/
int ima_file_mmap(struct file *file, unsigned long prot)
{
- int rc;
+ int rc = 0;
if (!file)
return 0;
if (prot & PROT_EXEC)
rc = process_measurement(file, file->f_dentry->d_name.name,
MAY_EXEC, FILE_MMAP);
- return 0;
+ return (ima_appraise & IMA_APPRAISE_ENFORCE) ? rc : 0;
}
EXPORT_SYMBOL_GPL(ima_file_mmap);
@@ -197,7 +225,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
rc = process_measurement(bprm->file, bprm->filename,
MAY_EXEC, BPRM_CHECK);
- return 0;
+ return (ima_appraise & IMA_APPRAISE_ENFORCE) ? rc : 0;
}
/**
@@ -218,7 +246,7 @@ int ima_file_check(struct file *file, int mask)
rc = process_measurement(file, file->f_dentry->d_name.name,
mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
FILE_CHECK);
- return 0;
+ return (ima_appraise & IMA_APPRAISE_ENFORCE) ? rc : 0;
}
EXPORT_SYMBOL_GPL(ima_file_check);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d8edff2..8ee301c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -25,7 +25,13 @@
#define IMA_FSMAGIC 0x0004
#define IMA_UID 0x0008
-enum ima_action { UNKNOWN = -1, DONT_MEASURE = 0, MEASURE };
+#define UNKNOWN 0
+#define MEASURE 1 /* same as IMA_MEASURE */
+#define DONT_MEASURE 2
+#define MEASURE_MASK 3
+#define APPRAISE 4 /* same as IMA_APPRAISE */
+#define DONT_APPRAISE 8
+#define APPRAISE_MASK 12
#define MAX_LSM_RULES 6
enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
@@ -34,7 +40,7 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
struct ima_measure_rule_entry {
struct list_head list;
- enum ima_action action;
+ int action;
unsigned int flags;
enum ima_hooks func;
int mask;
@@ -161,18 +167,28 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
* as elements in the list are never deleted, nor does the list
* change.)
*/
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
+int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
+ int flags)
{
struct ima_measure_rule_entry *entry;
+ int action = 0, actmask = flags | (flags << 1);
list_for_each_entry(entry, ima_measure, list) {
- bool rc;
- rc = ima_match_rules(entry, inode, func, mask);
- if (rc)
- return entry->action;
+ if (!(entry->action & actmask))
+ continue;
+
+ if (!ima_match_rules(entry, inode, func, mask))
+ continue;
+
+ action |= (entry->action & (IMA_APPRAISE | IMA_MEASURE));
+ actmask &= (entry->action & APPRAISE_MASK) ?
+ ~APPRAISE_MASK : ~MEASURE_MASK;
+ if (!actmask)
+ break;
}
- return 0;
+
+ return action;
}
/**
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7a25ece..295702d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -16,7 +16,11 @@
#include <crypto/sha.h>
/* iint cache flags */
-#define IMA_MEASURED 0x01
+#define IMA_MEASURE 1
+#define IMA_MEASURED 2
+#define IMA_APPRAISE 4
+#define IMA_APPRAISED 8
+#define IMA_COLLECTED 16
enum evm_ima_xattr_type {
IMA_XATTR_DIGEST = 0x01,
@@ -36,7 +40,7 @@ struct integrity_iint_cache {
u64 version; /* track inode changes */
unsigned char flags;
u8 digest[SHA1_DIGEST_SIZE];
- struct mutex mutex; /* protects: version, flags, digest */
+ enum integrity_status ima_status;
enum integrity_status evm_status;
};
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 04/12] ima: integrity appraisal extension
2012-03-21 18:54 ` [PATCH v3 04/12] ima: integrity appraisal extension Mimi Zohar
@ 2012-03-22 14:28 ` Kasatkin, Dmitry
0 siblings, 0 replies; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 14:28 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
On Wed, Mar 21, 2012 at 8:54 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> IMA currently maintains an integrity measurement list used to assert the
> integrity of the running system to a third party. The IMA-appraisal
> extension adds local integrity validation and enforcement of the
> measurement against a "good" value stored as an extended attribute
> 'security.ima'. The initial methods for validating 'security.ima' are
> hashed based, which provides file data integrity, and digital signature
> based, which in addition to providing file data integrity, provides
> authenticity.
>
> This patch creates and maintains the 'security.ima' xattr, containing
> the file data hash measurement. Protection of the xattr is provided by
> EVM, if enabled and configured.
>
> Based on policy, IMA calls evm_verifyxattr() to verify a file's metadata
> integrity and, assuming success, compares the file's current hash value
> with the one stored as an extended attribute in 'security.ima'.
>
> Changelog v3:
> - change appraisal default for filesystems without xattr support to fail
>
> Changelog v2:
> - fix audit msg 'res' value
> - removed unused 'ima_appraise=' values
>
> Changelog v1:
> - removed unused iint mutex (Dmitry Kasatkin)
> - setattr hook must not reset appraised (Dmitry Kasatkin)
> - evm_verifyxattr() now differentiates between no 'security.evm' xattr
> (INTEGRITY_NOLABEL) and no EVM 'protected' xattrs included in the
> 'security.evm' (INTEGRITY_NOXATTRS).
> - replace hash_status with ima_status (Dmitry Kasatkin)
> - re-initialize slab element ima_status on free (Dmitry Kasatkin)
> - include 'security.ima' in EVM if CONFIG_IMA_APPRAISE, not CONFIG_IMA
> - merged half "ima: ima_must_appraise_or_measure API change" (Dmitry Kasatkin)
> - removed unnecessary error variable in process_measurement() (Dmitry Kasatkin)
> - use ima_inode_post_setattr() stub function, if IMA_APPRAISE not configured
> (moved ima_inode_post_setattr() to ima_appraise.c)
> - make sure ima_collect_measurement() can read file
>
> Changelog:
> - add 'iint' to evm_verifyxattr() call (Dimitry Kasatkin)
> - fix the race condition between chmod, which takes the i_mutex and then
> iint->mutex, and ima_file_free() and process_measurement(), which take
> the locks in the reverse order, by eliminating iint->mutex. (Dmitry Kasatkin)
> - cleanup of ima_appraise_measurement() (Dmitry Kasatkin)
> - changes as a result of the iint not allocated for all regular files, but
> only for those measured/appraised.
> - don't try to appraise new/empty files
> - expanded ima_appraisal description in ima/Kconfig
> - IMA appraise definitions required even if IMA_APPRAISE not enabled
> - add return value to ima_must_appraise() stub
> - unconditionally set status = INTEGRITY_PASS *after* testing status,
> not before. (Found by Joe Perches)
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> ---
> Documentation/kernel-parameters.txt | 4 +
> include/linux/xattr.h | 3 +
> security/integrity/evm/evm_main.c | 3 +
> security/integrity/iint.c | 3 +-
> security/integrity/ima/Kconfig | 15 +++
> security/integrity/ima/Makefile | 2 +
> security/integrity/ima/ima.h | 37 +++++++-
> security/integrity/ima/ima_api.c | 50 +++++++---
> security/integrity/ima/ima_appraise.c | 168 +++++++++++++++++++++++++++++++++
> security/integrity/ima/ima_crypto.c | 8 ++-
> security/integrity/ima/ima_main.c | 78 ++++++++++-----
> security/integrity/ima/ima_policy.c | 32 +++++--
> security/integrity/integrity.h | 8 +-
> 13 files changed, 358 insertions(+), 53 deletions(-)
> create mode 100644 security/integrity/ima/ima_appraise.c
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 033d4e6..a86765d 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1004,6 +1004,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> ihash_entries= [KNL]
> Set number of hash buckets for inode cache.
>
> + ima_appraise= [IMA] appraise integrity measurements
> + Format: { "off" | "enforce" | "fix" }
> + default: "enforce"
> +
> ima_audit= [IMA]
> Format: { "0" | "1" }
> 0 -- integrity auditing messages. (Default)
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index e5d1220..77a3e68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -33,6 +33,9 @@
> #define XATTR_EVM_SUFFIX "evm"
> #define XATTR_NAME_EVM XATTR_SECURITY_PREFIX XATTR_EVM_SUFFIX
>
> +#define XATTR_IMA_SUFFIX "ima"
> +#define XATTR_NAME_IMA XATTR_SECURITY_PREFIX XATTR_IMA_SUFFIX
> +
> #define XATTR_SELINUX_SUFFIX "selinux"
> #define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 8901501..eb54845 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -34,6 +34,9 @@ char *evm_config_xattrnames[] = {
> #ifdef CONFIG_SECURITY_SMACK
> XATTR_NAME_SMACK,
> #endif
> +#ifdef CONFIG_IMA_APPRAISE
> + XATTR_NAME_IMA,
> +#endif
> XATTR_NAME_CAPS,
> NULL
> };
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 399641c..e600986 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -74,6 +74,7 @@ static void iint_free(struct integrity_iint_cache *iint)
> {
> iint->version = 0;
> iint->flags = 0UL;
> + iint->ima_status = INTEGRITY_UNKNOWN;
> iint->evm_status = INTEGRITY_UNKNOWN;
> kmem_cache_free(iint_cache, iint);
> }
> @@ -157,7 +158,7 @@ static void init_once(void *foo)
> memset(iint, 0, sizeof *iint);
> iint->version = 0;
> iint->flags = 0UL;
> - mutex_init(&iint->mutex);
> + iint->ima_status = INTEGRITY_UNKNOWN;
> iint->evm_status = INTEGRITY_UNKNOWN;
> }
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 35664fe..b7465a1 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -54,3 +54,18 @@ config IMA_LSM_RULES
> default y
> help
> Disabling this option will disregard LSM based policy rules.
> +
> +config IMA_APPRAISE
> + bool "Appraise integrity measurements"
> + depends on IMA
> + default n
> + help
> + This option enables local measurement integrity appraisal.
> + It requires the system to be labeled with a security extended
> + attribute containing the file hash measurement. To protect
> + the security extended attributes from offline attack, enable
> + and configure EVM.
> +
> + For more information on integrity appraisal refer to:
> + <http://linux-ima.sourceforge.net>
> + If unsure, say N.
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index 5690c02..bd31516 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -7,3 +7,5 @@ obj-$(CONFIG_IMA) += ima.o
>
> ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> ima_policy.o ima_audit.o
> +
> +ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 3ccf7ac..d5bf463 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -40,6 +40,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> extern int ima_initialized;
> extern int ima_used_chip;
> extern char *ima_hash;
> +extern int ima_appraise;
>
> /* IMA inode template definition */
> struct ima_template_data {
> @@ -98,6 +99,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
> }
>
> /* LIM API function definitions */
> +int ima_must_appraise_or_measure(struct inode *inode, int mask, int function);
> int ima_must_measure(struct inode *inode, int mask, int function);
> int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct file *file);
> @@ -114,14 +116,45 @@ struct integrity_iint_cache *integrity_iint_insert(struct inode *inode);
> struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>
> /* IMA policy related functions */
> -enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
> +enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK, POST_SETATTR };
>
> -int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
> +int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
> + int flags);
> void ima_init_policy(void);
> void ima_update_policy(void);
> ssize_t ima_parse_add_rule(char *);
> void ima_delete_rules(void);
>
> +/* Appraise integrity measurements */
> +#define IMA_APPRAISE_ENFORCE 0x01
> +#define IMA_APPRAISE_FIX 0x02
> +
> +#ifdef CONFIG_IMA_APPRAISE
> +int ima_appraise_measurement(struct integrity_iint_cache *iint,
> + struct file *file, const unsigned char *filename);
> +int ima_must_appraise(struct inode *inode, enum ima_hooks func, int mask);
> +void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
> +
> +#else
> +static inline int ima_appraise_measurement(struct integrity_iint_cache *iint,
> + struct file *file,
> + const unsigned char *filename)
> +{
> + return INTEGRITY_UNKNOWN;
> +}
> +
> +static inline int ima_must_appraise(struct inode *inode,
> + enum ima_hooks func, int mask)
> +{
> + return 0;
> +}
> +
> +static inline void ima_update_xattr(struct integrity_iint_cache *iint,
> + struct file *file)
> +{
> +}
> +#endif
> +
> /* LSM based policy rules require audit */
> #ifdef CONFIG_IMA_LSM_RULES
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 88a2788..55deeb1 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -9,13 +9,17 @@
> * License.
> *
> * File: ima_api.c
> - * Implements must_measure, collect_measurement, store_measurement,
> - * and store_template.
> + * Implements must_appraise_or_measure, collect_measurement,
> + * appraise_measurement, store_measurement and store_template.
> */
> #include <linux/module.h>
> #include <linux/slab.h>
> -
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/xattr.h>
> +#include <linux/evm.h>
> #include "ima.h"
> +
> static const char *IMA_TEMPLATE_NAME = "ima";
>
> /*
> @@ -93,7 +97,7 @@ err_out:
> }
>
> /**
> - * ima_must_measure - measure decision based on policy.
> + * ima_must_appraise_or_measure - appraise & measure decision based on policy.
> * @inode: pointer to inode to measure
> * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXECUTE)
> * @function: calling function (FILE_CHECK, BPRM_CHECK, FILE_MMAP)
> @@ -105,15 +109,22 @@ err_out:
> * mask: contains the permission mask
> * fsmagic: hex value
> *
> - * Return 0 to measure. For matching a DONT_MEASURE policy, no policy,
> - * or other error, return an error code.
> -*/
> -int ima_must_measure(struct inode *inode, int mask, int function)
> + * Returns IMA_MEASURE, IMA_APPRAISE mask.
> + *
> + */
> +int ima_must_appraise_or_measure(struct inode *inode, int mask, int function)
> {
> - int must_measure;
> + int flags = IMA_MEASURE | IMA_APPRAISE;
> +
> + if (!ima_appraise)
> + flags &= ~IMA_APPRAISE;
> +
> + return ima_match_policy(inode, function, mask, flags);
> +}
>
> - must_measure = ima_match_policy(inode, function, mask);
> - return must_measure ? 0 : -EACCES;
> +int ima_must_measure(struct inode *inode, int mask, int function)
> +{
> + return ima_match_policy(inode, function, mask, IMA_MEASURE);
> }
>
> /*
> @@ -129,16 +140,24 @@ int ima_must_measure(struct inode *inode, int mask, int function)
> int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct file *file)
> {
> - int result = -EEXIST;
> + struct inode *inode = file->f_dentry->d_inode;
> + const char *filename = file->f_dentry->d_name.name;
> + int result = 0;
>
> - if (!(iint->flags & IMA_MEASURED)) {
> + if (!(iint->flags & IMA_COLLECTED)) {
> u64 i_version = file->f_dentry->d_inode->i_version;
>
> memset(iint->digest, 0, IMA_DIGEST_SIZE);
> result = ima_calc_hash(file, iint->digest);
> - if (!result)
> + if (!result) {
> iint->version = i_version;
> + iint->flags |= IMA_COLLECTED;
> + }
> }
> + if (result)
> + integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> + filename, "collect_data", "failed",
> + result, 0);
> return result;
> }
>
> @@ -167,6 +186,9 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
> struct ima_template_entry *entry;
> int violation = 0;
>
> + if (iint->flags & IMA_MEASURED)
> + return;
> +
> entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> if (!entry) {
> integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> new file mode 100644
> index 0000000..4865f61
> --- /dev/null
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -0,0 +1,168 @@
> +/*
> + * Copyright (C) 2011 IBM Corporation
> + *
> + * Author:
> + * Mimi Zohar <zohar@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +#include <linux/module.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/xattr.h>
> +#include <linux/magic.h>
> +#include <linux/ima.h>
> +#include <linux/evm.h>
> +
> +#include "ima.h"
> +
> +static int __init default_appraise_setup(char *str)
> +{
> + if (strncmp(str, "off", 3) == 0)
> + ima_appraise = 0;
> + else if (strncmp(str, "fix", 3) == 0)
> + ima_appraise = IMA_APPRAISE_FIX;
> + return 1;
> +}
> +
> +__setup("ima_appraise=", default_appraise_setup);
> +
> +/*
> + * ima_must_appraise - set appraise flag
> + *
> + * Return 1 to appraise
> + */
> +int ima_must_appraise(struct inode *inode, enum ima_hooks func, int mask)
> +{
> + return 0;
> +}
> +
> +static void ima_fix_xattr(struct dentry *dentry,
> + struct integrity_iint_cache *iint)
> +{
> + iint->digest[0] = IMA_XATTR_DIGEST;
> + __vfs_setxattr_noperm(dentry, XATTR_NAME_IMA,
> + iint->digest, IMA_DIGEST_SIZE + 1, 0);
> +}
> +
> +/*
> + * ima_appraise_measurement - appraise file measurement
> + *
> + * Call evm_verifyxattr() to verify the integrity of 'security.ima'.
> + * Assuming success, compare the xattr hash with the collected measurement.
> + *
> + * Return 0 on success, error code otherwise
> + */
> +int ima_appraise_measurement(struct integrity_iint_cache *iint,
> + struct file *file, const unsigned char *filename)
> +{
> + struct dentry *dentry = file->f_dentry;
> + struct inode *inode = dentry->d_inode;
> + u8 xattr_value[IMA_DIGEST_SIZE];
> + enum integrity_status status = INTEGRITY_UNKNOWN;
> + const char *op = "appraise_data";
> + char *cause = "unknown";
> + int rc;
> +
> + if (!ima_appraise)
> + return 0;
> + if (!inode->i_op->getxattr)
> + return INTEGRITY_UNKNOWN;
> +
> + if (iint->flags & IMA_APPRAISED)
> + return iint->ima_status;
> +
> + rc = inode->i_op->getxattr(dentry, XATTR_NAME_IMA, xattr_value,
> + IMA_DIGEST_SIZE);
> + if (rc <= 0) {
> + if (rc && rc != -ENODATA)
> + goto out;
> +
> + cause = "missing-hash";
> + status =
> + (inode->i_size == 0) ? INTEGRITY_PASS : INTEGRITY_NOLABEL;
> + goto out;
> + }
> +
> + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> + if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
> + if ((status == INTEGRITY_NOLABEL)
> + || (status == INTEGRITY_NOXATTRS))
> + cause = "missing-HMAC";
> + else if (status == INTEGRITY_FAIL)
> + cause = "invalid-HMAC";
> + goto out;
> + }
> +
> + rc = memcmp(xattr_value, iint->digest, IMA_DIGEST_SIZE);
> + if (rc) {
> + status = INTEGRITY_FAIL;
> + cause = "invalid-hash";
> + print_hex_dump_bytes("security.ima: ", DUMP_PREFIX_NONE,
> + xattr_value, IMA_DIGEST_SIZE);
> + print_hex_dump_bytes("collected: ", DUMP_PREFIX_NONE,
> + iint->digest, IMA_DIGEST_SIZE);
> + goto out;
> + }
> + status = INTEGRITY_PASS;
> + iint->flags |= IMA_APPRAISED;
> +out:
> + if (status != INTEGRITY_PASS) {
> + if (ima_appraise & IMA_APPRAISE_FIX) {
> + ima_fix_xattr(dentry, iint);
> + status = INTEGRITY_PASS;
> + }
> + integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> + op, cause, rc, 0);
> + }
> + iint->ima_status = status;
> + return status;
> +}
> +
> +/*
> + * ima_update_xattr - update 'security.ima' hash value
> + */
> +void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
> +{
> + struct dentry *dentry = file->f_dentry;
> + int rc = 0;
> +
> + rc = ima_collect_measurement(iint, file);
> + if (rc < 0)
> + return;
> + ima_fix_xattr(dentry, iint);
> +}
> +
> +/**
> + * ima_inode_post_setattr - reflect file metadata changes
> + * @dentry: pointer to the affected dentry
> + *
> + * Changes to a dentry's metadata might result in needing to appraise.
> + *
> + * This function is called from notify_change(), which expects the caller
> + * to lock the inode's i_mutex.
> + */
> +void ima_inode_post_setattr(struct dentry *dentry)
> +{
> + struct inode *inode = dentry->d_inode;
> + struct integrity_iint_cache *iint;
> + int must_appraise, rc;
> +
> + if (!ima_initialized || !ima_appraise || !S_ISREG(inode->i_mode)
> + || !inode->i_op->removexattr)
> + return;
> +
> + must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
> + iint = integrity_iint_find(inode);
> + if (iint) {
> + if (must_appraise)
> + iint->flags |= IMA_APPRAISE;
> + else
> + iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED);
> + }
> + if (!must_appraise)
> + rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
> + return;
> +}
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 9b3ade7..b21ee5b 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -48,7 +48,7 @@ int ima_calc_hash(struct file *file, char *digest)
> struct scatterlist sg[1];
> loff_t i_size, offset = 0;
> char *rbuf;
> - int rc;
> + int rc, read = 0;
>
> rc = init_desc(&desc);
> if (rc != 0)
> @@ -59,6 +59,10 @@ int ima_calc_hash(struct file *file, char *digest)
> rc = -ENOMEM;
> goto out;
> }
> + if (!(file->f_mode & FMODE_READ)) {
> + file->f_mode |= FMODE_READ;
> + read = 1;
> + }
> i_size = i_size_read(file->f_dentry->d_inode);
> while (offset < i_size) {
> int rbuf_len;
> @@ -80,6 +84,8 @@ int ima_calc_hash(struct file *file, char *digest)
> kfree(rbuf);
> if (!rc)
> rc = crypto_hash_final(&desc, digest);
> + if (read)
> + file->f_mode &= ~FMODE_READ;
> out:
> crypto_free_hash(desc.tfm);
> return rc;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 5b222eb..fba2f7b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -22,12 +22,19 @@
> #include <linux/mount.h>
> #include <linux/mman.h>
> #include <linux/slab.h>
> +#include <linux/xattr.h>
> #include <linux/ima.h>
>
> #include "ima.h"
>
> int ima_initialized;
>
> +#ifdef CONFIG_IMA_APPRAISE
> +int ima_appraise = IMA_APPRAISE_ENFORCE;
> +#else
> +int ima_appraise;
> +#endif
> +
> char *ima_hash = "sha1";
> static int __init hash_setup(char *str)
> {
> @@ -52,7 +59,7 @@ static void ima_rdwr_violation_check(struct file *file)
> struct dentry *dentry = file->f_path.dentry;
> struct inode *inode = dentry->d_inode;
> fmode_t mode = file->f_mode;
> - int rc;
> + int must_measure;
> bool send_tomtou = false, send_writers = false;
>
> if (!S_ISREG(inode->i_mode) || !ima_initialized)
> @@ -66,8 +73,8 @@ static void ima_rdwr_violation_check(struct file *file)
> goto out;
> }
>
> - rc = ima_must_measure(inode, MAY_READ, FILE_CHECK);
> - if (rc < 0)
> + must_measure = ima_must_measure(inode, MAY_READ, FILE_CHECK);
> + if (!must_measure)
> goto out;
>
> if (atomic_read(&inode->i_writecount) > 0)
> @@ -84,17 +91,21 @@ out:
> }
>
> static void ima_check_last_writer(struct integrity_iint_cache *iint,
> - struct inode *inode,
> - struct file *file)
> + struct inode *inode, struct file *file)
> {
> fmode_t mode = file->f_mode;
>
> - mutex_lock(&iint->mutex);
> - if (mode & FMODE_WRITE &&
> - atomic_read(&inode->i_writecount) == 1 &&
> - iint->version != inode->i_version)
> - iint->flags &= ~IMA_MEASURED;
> - mutex_unlock(&iint->mutex);
> + if (!(mode & FMODE_WRITE))
> + return;
> +
> + mutex_lock(&inode->i_mutex);
> + if (atomic_read(&inode->i_writecount) == 1 &&
> + iint->version != inode->i_version) {
> + iint->flags &= ~(IMA_COLLECTED | IMA_APPRAISED | IMA_MEASURED);
> + if (iint->flags & IMA_APPRAISE)
> + ima_update_xattr(iint, file);
> + }
> + mutex_unlock(&inode->i_mutex);
> }
>
> /**
> @@ -123,14 +134,17 @@ static int process_measurement(struct file *file, const unsigned char *filename,
> {
> struct inode *inode = file->f_dentry->d_inode;
> struct integrity_iint_cache *iint;
> - int rc = 0;
> + int rc = -ENOMEM, action, must_appraise;
>
> if (!ima_initialized || !S_ISREG(inode->i_mode))
> return 0;
>
> - rc = ima_must_measure(inode, mask, function);
> - if (rc != 0)
> - return rc;
> + /* Determine if in appraise/measurement policy,
> + * returns IMA_MEASURE, IMA_APPRAISE bitmask. */
> + action = ima_must_appraise_or_measure(inode, mask, function);
> + if (!action)
> + return 0;
> +
> retry:
> iint = integrity_iint_find(inode);
> if (!iint) {
> @@ -140,18 +154,32 @@ retry:
> return rc;
> }
>
> - mutex_lock(&iint->mutex);
> + must_appraise = action & IMA_APPRAISE;
>
> - rc = iint->flags & IMA_MEASURED ? 1 : 0;
> - if (rc != 0)
> + mutex_lock(&inode->i_mutex);
> +
> + /* Determine if already appraised/measured based on bitmask
> + * (IMA_MEASURE, IMA_MEASURED, IMA_APPRAISE, IMA_APPRAISED) */
> + iint->flags |= action;
> + action &= ~((iint->flags & (IMA_MEASURED | IMA_APPRAISED)) >> 1);
> +
> + /* Nothing to do, just return existing appraised status */
> + if (!action) {
> + if (iint->flags & IMA_APPRAISED)
> + rc = iint->ima_status;
> goto out;
> + }
>
> rc = ima_collect_measurement(iint, file);
> - if (!rc)
> + if (rc != 0)
> + goto out;
> + if (action & IMA_MEASURE)
> ima_store_measurement(iint, file, filename);
> + if (action & IMA_APPRAISE)
> + rc = ima_appraise_measurement(iint, file, filename);
> out:
> - mutex_unlock(&iint->mutex);
> - return rc;
> + mutex_unlock(&inode->i_mutex);
> + return (rc && must_appraise) ? -EACCES : 0;
> }
>
> /**
> @@ -167,14 +195,14 @@ out:
> */
> int ima_file_mmap(struct file *file, unsigned long prot)
> {
> - int rc;
> + int rc = 0;
>
> if (!file)
> return 0;
> if (prot & PROT_EXEC)
> rc = process_measurement(file, file->f_dentry->d_name.name,
> MAY_EXEC, FILE_MMAP);
> - return 0;
> + return (ima_appraise & IMA_APPRAISE_ENFORCE) ? rc : 0;
> }
> EXPORT_SYMBOL_GPL(ima_file_mmap);
>
> @@ -197,7 +225,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
>
> rc = process_measurement(bprm->file, bprm->filename,
> MAY_EXEC, BPRM_CHECK);
> - return 0;
> + return (ima_appraise & IMA_APPRAISE_ENFORCE) ? rc : 0;
> }
>
> /**
> @@ -218,7 +246,7 @@ int ima_file_check(struct file *file, int mask)
> rc = process_measurement(file, file->f_dentry->d_name.name,
> mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
> FILE_CHECK);
> - return 0;
> + return (ima_appraise & IMA_APPRAISE_ENFORCE) ? rc : 0;
> }
> EXPORT_SYMBOL_GPL(ima_file_check);
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index d8edff2..8ee301c 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -25,7 +25,13 @@
> #define IMA_FSMAGIC 0x0004
> #define IMA_UID 0x0008
>
> -enum ima_action { UNKNOWN = -1, DONT_MEASURE = 0, MEASURE };
> +#define UNKNOWN 0
> +#define MEASURE 1 /* same as IMA_MEASURE */
> +#define DONT_MEASURE 2
> +#define MEASURE_MASK 3
> +#define APPRAISE 4 /* same as IMA_APPRAISE */
> +#define DONT_APPRAISE 8
> +#define APPRAISE_MASK 12
>
> #define MAX_LSM_RULES 6
> enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
> @@ -34,7 +40,7 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
>
> struct ima_measure_rule_entry {
> struct list_head list;
> - enum ima_action action;
> + int action;
> unsigned int flags;
> enum ima_hooks func;
> int mask;
> @@ -161,18 +167,28 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> * as elements in the list are never deleted, nor does the list
> * change.)
> */
> -int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
> +int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
> + int flags)
> {
> struct ima_measure_rule_entry *entry;
> + int action = 0, actmask = flags | (flags << 1);
>
> list_for_each_entry(entry, ima_measure, list) {
> - bool rc;
>
> - rc = ima_match_rules(entry, inode, func, mask);
> - if (rc)
> - return entry->action;
> + if (!(entry->action & actmask))
> + continue;
> +
> + if (!ima_match_rules(entry, inode, func, mask))
> + continue;
> +
> + action |= (entry->action & (IMA_APPRAISE | IMA_MEASURE));
> + actmask &= (entry->action & APPRAISE_MASK) ?
> + ~APPRAISE_MASK : ~MEASURE_MASK;
> + if (!actmask)
> + break;
> }
> - return 0;
> +
> + return action;
> }
>
> /**
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 7a25ece..295702d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -16,7 +16,11 @@
> #include <crypto/sha.h>
>
> /* iint cache flags */
> -#define IMA_MEASURED 0x01
> +#define IMA_MEASURE 1
> +#define IMA_MEASURED 2
> +#define IMA_APPRAISE 4
> +#define IMA_APPRAISED 8
> +#define IMA_COLLECTED 16
>
> enum evm_ima_xattr_type {
> IMA_XATTR_DIGEST = 0x01,
> @@ -36,7 +40,7 @@ struct integrity_iint_cache {
> u64 version; /* track inode changes */
> unsigned char flags;
> u8 digest[SHA1_DIGEST_SIZE];
> - struct mutex mutex; /* protects: version, flags, digest */
> + enum integrity_status ima_status;
> enum integrity_status evm_status;
> };
>
> --
> 1.7.6.5
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 05/12] ima: add appraise action keywords and default rules
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (3 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 04/12] ima: integrity appraisal extension Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-22 14:27 ` Kasatkin, Dmitry
2012-03-21 18:54 ` [PATCH v3 06/12] ima: allocating iint improvements Mimi Zohar
` (6 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Mimi Zohar
Unlike the IMA measurement policy, the appraise policy can not be dependent
on runtime process information, such as the task uid, as the 'security.ima'
xattr is written on file close and must be updated each time the file changes,
regardless of the current task uid.
This patch extends the policy language with 'fowner', defines an appraise
policy, which appraises all files owned by root, and defines 'ima_appraise_tcb',
a new boot command line option, to enable the appraise policy.
Changelog v3:
- separate the measure from the appraise rules in order to support measuring
without appraising and appraising without measuring.
- change appraisal default for filesystems without xattr support to fail
- update default appraise policy for cgroups
Changelog v1:
- don't appraise RAMFS (Dmitry Kasatkin)
- merged rest of "ima: ima_must_appraise_or_measure API change" commit
(Dmtiry Kasatkin)
ima_must_appraise_or_measure() called ima_match_policy twice, which
searched the policy for a matching rule. Once for a matching measurement
rule and subsequently for an appraisal rule. Searching the policy twice
is unnecessary overhead, which could be noticeable with a large policy.
The new version of ima_must_appraise_or_measure() does everything in a
single iteration using a new version of ima_match_policy(). It returns
IMA_MEASURE, IMA_APPRAISE mask.
With the use of action mask only one efficient matching function
is enough. Removed other specific versions of matching functions.
Changelog:
- change 'owner' to 'fowner' to conform to the new LSM conditions posted by
Roberto Sassu.
- fix calls to ima_log_string()
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
Documentation/ABI/testing/ima_policy | 25 +++++-
Documentation/kernel-parameters.txt | 4 +
security/integrity/ima/ima_appraise.c | 5 +-
security/integrity/ima/ima_policy.c | 149 ++++++++++++++++++++++++---------
4 files changed, 139 insertions(+), 44 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 6cd6dae..dcff822 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -12,11 +12,14 @@ Description:
then closing the file. The new policy takes effect after
the file ima/policy is closed.
+ IMA appraisal, if configured, uses these file measurements
+ for local measurement appraisal.
+
rule format: action [condition ...]
- action: measure | dont_measure
+ action: measure | dont_measure | appraise | dont_appraise
condition:= base | lsm
- base: [[func=] [mask=] [fsmagic=] [uid=]]
+ base: [[func=] [mask=] [fsmagic=] [uid=] [fowner]]
lsm: [[subj_user=] [subj_role=] [subj_type=]
[obj_user=] [obj_role=] [obj_type=]]
@@ -24,36 +27,50 @@ Description:
mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
fsmagic:= hex value
uid:= decimal value
+ fowner:=decimal value
lsm: are LSM specific
default policy:
# PROC_SUPER_MAGIC
dont_measure fsmagic=0x9fa0
+ dont_appraise fsmagic=0x9fa0
# SYSFS_MAGIC
dont_measure fsmagic=0x62656572
+ dont_appraise fsmagic=0x62656572
# DEBUGFS_MAGIC
dont_measure fsmagic=0x64626720
+ dont_appraise fsmagic=0x64626720
# TMPFS_MAGIC
dont_measure fsmagic=0x01021994
+ dont_appraise fsmagic=0x01021994
+ # RAMFS_MAGIC
+ dont_measure fsmagic=0x858458f6
+ dont_appraise fsmagic=0x858458f6
# SECURITYFS_MAGIC
dont_measure fsmagic=0x73636673
+ dont_appraise fsmagic=0x73636673
measure func=BPRM_CHECK
measure func=FILE_MMAP mask=MAY_EXEC
measure func=FILE_CHECK mask=MAY_READ uid=0
+ appraise fowner=0
The default policy measures all executables in bprm_check,
all files mmapped executable in file_mmap, and all files
- open for read by root in do_filp_open.
+ open for read by root in do_filp_open. The default appraisal
+ policy appraises all files owned by root.
Examples of LSM specific definitions:
SELinux:
# SELINUX_MAGIC
- dont_measure fsmagic=0xF97CFF8C
+ dont_measure fsmagic=0xf97cff8c
+ dont_appraise fsmagic=0xf97cff8c
dont_measure obj_type=var_log_t
+ dont_appraise obj_type=var_log_t
dont_measure obj_type=auditd_log_t
+ dont_appraise obj_type=auditd_log_t
measure subj_user=system_u func=FILE_CHECK mask=MAY_READ
measure subj_role=system_r func=FILE_CHECK mask=MAY_READ
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a86765d..6c00491 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1008,6 +1008,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
Format: { "off" | "enforce" | "fix" }
default: "enforce"
+ ima_appraise_tcb [IMA]
+ The builtin appraise policy appraises all files
+ owned by uid=0.
+
ima_audit= [IMA]
Format: { "0" | "1" }
0 -- integrity auditing messages. (Default)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 4865f61..681cb6e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -36,7 +36,10 @@ __setup("ima_appraise=", default_appraise_setup);
*/
int ima_must_appraise(struct inode *inode, enum ima_hooks func, int mask)
{
- return 0;
+ if (!ima_appraise)
+ return 0;
+
+ return ima_match_policy(inode, func, mask, IMA_APPRAISE);
}
static void ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8ee301c..238aa2b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -24,6 +24,7 @@
#define IMA_MASK 0x0002
#define IMA_FSMAGIC 0x0004
#define IMA_UID 0x0008
+#define IMA_FOWNER 0x0010
#define UNKNOWN 0
#define MEASURE 1 /* same as IMA_MEASURE */
@@ -38,7 +39,7 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE
};
-struct ima_measure_rule_entry {
+struct ima_rule_entry {
struct list_head list;
int action;
unsigned int flags;
@@ -46,6 +47,7 @@ struct ima_measure_rule_entry {
int mask;
unsigned long fsmagic;
uid_t uid;
+ uid_t fowner;
struct {
void *rule; /* LSM file metadata specific */
int type; /* audit type */
@@ -54,7 +56,7 @@ struct ima_measure_rule_entry {
/*
* Without LSM specific knowledge, the default policy can only be
- * written in terms of .action, .func, .mask, .fsmagic, and .uid
+ * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
*/
/*
@@ -63,7 +65,7 @@ struct ima_measure_rule_entry {
* normal users can easily run the machine out of memory simply building
* and running executables.
*/
-static struct ima_measure_rule_entry default_rules[] = {
+static struct ima_rule_entry default_rules[] = {
{.action = DONT_MEASURE,.fsmagic = PROC_SUPER_MAGIC,.flags = IMA_FSMAGIC},
{.action = DONT_MEASURE,.fsmagic = SYSFS_MAGIC,.flags = IMA_FSMAGIC},
{.action = DONT_MEASURE,.fsmagic = DEBUGFS_MAGIC,.flags = IMA_FSMAGIC},
@@ -79,19 +81,39 @@ static struct ima_measure_rule_entry default_rules[] = {
.flags = IMA_FUNC | IMA_MASK | IMA_UID},
};
-static LIST_HEAD(measure_default_rules);
-static LIST_HEAD(measure_policy_rules);
-static struct list_head *ima_measure;
+static struct ima_rule_entry default_appraise_rules[] = {
+ {.action = DONT_APPRAISE,.fsmagic = PROC_SUPER_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_APPRAISE,.fsmagic = SYSFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_APPRAISE,.fsmagic = DEBUGFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_APPRAISE,.fsmagic = RAMFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_APPRAISE,.fsmagic = SECURITYFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_APPRAISE,.fsmagic = SELINUX_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_APPRAISE,.fsmagic = CGROUP_SUPER_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = APPRAISE,.fowner = 0,.flags = IMA_FOWNER},
+};
+
+static LIST_HEAD(ima_default_rules);
+static LIST_HEAD(ima_policy_rules);
+static struct list_head *ima_rules;
-static DEFINE_MUTEX(ima_measure_mutex);
+static DEFINE_MUTEX(ima_rules_mutex);
static bool ima_use_tcb __initdata;
-static int __init default_policy_setup(char *str)
+static int __init default_measure_policy_setup(char *str)
{
ima_use_tcb = 1;
return 1;
}
-__setup("ima_tcb", default_policy_setup);
+__setup("ima_tcb", default_measure_policy_setup);
+
+static bool ima_use_appraise_tcb __initdata;
+static int __init default_appraise_policy_setup(char *str)
+{
+ ima_use_appraise_tcb = 1;
+ return 1;
+}
+__setup("ima_appraise_tcb", default_appraise_policy_setup);
/**
* ima_match_rules - determine whether an inode matches the measure rule.
@@ -102,7 +124,7 @@ __setup("ima_tcb", default_policy_setup);
*
* Returns true on rule match, false on failure.
*/
-static bool ima_match_rules(struct ima_measure_rule_entry *rule,
+static bool ima_match_rules(struct ima_rule_entry *rule,
struct inode *inode, enum ima_hooks func, int mask)
{
struct task_struct *tsk = current;
@@ -118,6 +140,8 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
return false;
if ((rule->flags & IMA_UID) && rule->uid != cred->uid)
return false;
+ if ((rule->flags & IMA_FOWNER) && rule->fowner != inode->i_uid)
+ return false;
for (i = 0; i < MAX_LSM_RULES; i++) {
int rc = 0;
u32 osid, sid;
@@ -170,10 +194,10 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
int flags)
{
- struct ima_measure_rule_entry *entry;
+ struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
- list_for_each_entry(entry, ima_measure, list) {
+ list_for_each_entry(entry, ima_rules, list) {
if (!(entry->action & actmask))
continue;
@@ -194,22 +218,31 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
/**
* ima_init_policy - initialize the default measure rules.
*
- * ima_measure points to either the measure_default_rules or the
- * the new measure_policy_rules.
+ * ima_rules points to either the ima_default_rules or the
+ * the new ima_policy_rules.
*/
void __init ima_init_policy(void)
{
- int i, entries;
+ int i, measure_entries, appraise_entries;
/* if !ima_use_tcb set entries = 0 so we load NO default rules */
- if (ima_use_tcb)
- entries = ARRAY_SIZE(default_rules);
- else
- entries = 0;
-
- for (i = 0; i < entries; i++)
- list_add_tail(&default_rules[i].list, &measure_default_rules);
- ima_measure = &measure_default_rules;
+ measure_entries = ima_use_tcb ? ARRAY_SIZE(default_rules) : 0;
+ appraise_entries = ima_use_appraise_tcb ?
+ ARRAY_SIZE(default_appraise_rules) : 0;
+
+ for (i = 0; i < measure_entries + appraise_entries; i++) {
+ if (i < measure_entries)
+ list_add_tail(&default_rules[i].list,
+ &ima_default_rules);
+ else {
+ int j = i - measure_entries;
+
+ list_add_tail(&default_appraise_rules[j].list,
+ &ima_default_rules);
+ }
+ }
+
+ ima_rules = &ima_default_rules;
}
/**
@@ -226,8 +259,8 @@ void ima_update_policy(void)
int result = 1;
int audit_info = 0;
- if (ima_measure == &measure_default_rules) {
- ima_measure = &measure_policy_rules;
+ if (ima_rules == &ima_default_rules) {
+ ima_rules = &ima_policy_rules;
cause = "complete";
result = 0;
}
@@ -238,14 +271,17 @@ void ima_update_policy(void)
enum {
Opt_err = -1,
Opt_measure = 1, Opt_dont_measure,
+ Opt_appraise, Opt_dont_appraise,
Opt_obj_user, Opt_obj_role, Opt_obj_type,
Opt_subj_user, Opt_subj_role, Opt_subj_type,
- Opt_func, Opt_mask, Opt_fsmagic, Opt_uid
+ Opt_func, Opt_mask, Opt_fsmagic, Opt_uid, Opt_fowner
};
static match_table_t policy_tokens = {
{Opt_measure, "measure"},
{Opt_dont_measure, "dont_measure"},
+ {Opt_appraise, "appraise"},
+ {Opt_dont_appraise, "dont_appraise"},
{Opt_obj_user, "obj_user=%s"},
{Opt_obj_role, "obj_role=%s"},
{Opt_obj_type, "obj_type=%s"},
@@ -256,10 +292,11 @@ static match_table_t policy_tokens = {
{Opt_mask, "mask=%s"},
{Opt_fsmagic, "fsmagic=%s"},
{Opt_uid, "uid=%s"},
+ {Opt_fowner, "fowner=%s"},
{Opt_err, NULL}
};
-static int ima_lsm_rule_init(struct ima_measure_rule_entry *entry,
+static int ima_lsm_rule_init(struct ima_rule_entry *entry,
char *args, int lsm_rule, int audit_type)
{
int result;
@@ -283,7 +320,7 @@ static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
audit_log_format(ab, " ");
}
-static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
+static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
{
struct audit_buffer *ab;
char *p;
@@ -292,6 +329,7 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
entry->uid = -1;
+ entry->fowner = -1;
entry->action = UNKNOWN;
while ((p = strsep(&rule, " \t")) != NULL) {
substring_t args[MAX_OPT_ARGS];
@@ -320,11 +358,27 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
entry->action = DONT_MEASURE;
break;
+ case Opt_appraise:
+ ima_log_string(ab, "action", "appraise");
+
+ if (entry->action != UNKNOWN)
+ result = -EINVAL;
+
+ entry->action = APPRAISE;
+ break;
+ case Opt_dont_appraise:
+ ima_log_string(ab, "action", "dont_appraise");
+
+ if (entry->action != UNKNOWN)
+ result = -EINVAL;
+
+ entry->action = DONT_APPRAISE;
+ break;
case Opt_func:
ima_log_string(ab, "func", args[0].from);
if (entry->func)
- result = -EINVAL;
+ result = -EINVAL;
if (strcmp(args[0].from, "FILE_CHECK") == 0)
entry->func = FILE_CHECK;
@@ -389,6 +443,23 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
entry->flags |= IMA_UID;
}
break;
+ case Opt_fowner:
+ ima_log_string(ab, "fowner", args[0].from);
+
+ if (entry->fowner != -1) {
+ result = -EINVAL;
+ break;
+ }
+
+ result = strict_strtoul(args[0].from, 10, &lnum);
+ if (!result) {
+ entry->fowner = (uid_t) lnum;
+ if (entry->fowner != lnum)
+ result = -EINVAL;
+ else
+ entry->flags |= IMA_FOWNER;
+ }
+ break;
case Opt_obj_user:
ima_log_string(ab, "obj_user", args[0].from);
result = ima_lsm_rule_init(entry, args[0].from,
@@ -440,7 +511,7 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
}
/**
- * ima_parse_add_rule - add a rule to measure_policy_rules
+ * ima_parse_add_rule - add a rule to ima_policy_rules
* @rule - ima measurement policy rule
*
* Uses a mutex to protect the policy list from multiple concurrent writers.
@@ -450,12 +521,12 @@ ssize_t ima_parse_add_rule(char *rule)
{
const char *op = "update_policy";
char *p;
- struct ima_measure_rule_entry *entry;
+ struct ima_rule_entry *entry;
ssize_t result, len;
int audit_info = 0;
/* Prevent installed policy from changing */
- if (ima_measure != &measure_default_rules) {
+ if (ima_rules != &ima_default_rules) {
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
NULL, op, "already exists",
-EACCES, audit_info);
@@ -488,9 +559,9 @@ ssize_t ima_parse_add_rule(char *rule)
return result;
}
- mutex_lock(&ima_measure_mutex);
- list_add_tail(&entry->list, &measure_policy_rules);
- mutex_unlock(&ima_measure_mutex);
+ mutex_lock(&ima_rules_mutex);
+ list_add_tail(&entry->list, &ima_policy_rules);
+ mutex_unlock(&ima_rules_mutex);
return len;
}
@@ -498,12 +569,12 @@ ssize_t ima_parse_add_rule(char *rule)
/* ima_delete_rules called to cleanup invalid policy */
void ima_delete_rules(void)
{
- struct ima_measure_rule_entry *entry, *tmp;
+ struct ima_rule_entry *entry, *tmp;
- mutex_lock(&ima_measure_mutex);
- list_for_each_entry_safe(entry, tmp, &measure_policy_rules, list) {
+ mutex_lock(&ima_rules_mutex);
+ list_for_each_entry_safe(entry, tmp, &ima_policy_rules, list) {
list_del(&entry->list);
kfree(entry);
}
- mutex_unlock(&ima_measure_mutex);
+ mutex_unlock(&ima_rules_mutex);
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 05/12] ima: add appraise action keywords and default rules
2012-03-21 18:54 ` [PATCH v3 05/12] ima: add appraise action keywords and default rules Mimi Zohar
@ 2012-03-22 14:27 ` Kasatkin, Dmitry
0 siblings, 0 replies; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 14:27 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 22776 bytes --]
On Wed, Mar 21, 2012 at 8:54 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Unlike the IMA measurement policy, the appraise policy can not be dependent
> on runtime process information, such as the task uid, as the 'security.ima'
> xattr is written on file close and must be updated each time the file changes,
> regardless of the current task uid.
>
> This patch extends the policy language with 'fowner', defines an appraise
> policy, which appraises all files owned by root, and defines 'ima_appraise_tcb',
> a new boot command line option, to enable the appraise policy.
>
> Changelog v3:
> - separate the measure from the appraise rules in order to support measuring
> Â without appraising and appraising without measuring.
> - change appraisal default for filesystems without xattr support to fail
> - update default appraise policy for cgroups
>
> Changelog v1:
> - don't appraise RAMFS (Dmitry Kasatkin)
> - merged rest of "ima: ima_must_appraise_or_measure API change" commit
> Â (Dmtiry Kasatkin)
>
> Â ima_must_appraise_or_measure() called ima_match_policy twice, which
> Â searched the policy for a matching rule. Â Once for a matching measurement
> Â rule and subsequently for an appraisal rule. Searching the policy twice
> Â is unnecessary overhead, which could be noticeable with a large policy.
>
> Â The new version of ima_must_appraise_or_measure() does everything in a
> Â single iteration using a new version of ima_match_policy(). Â It returns
> Â IMA_MEASURE, IMA_APPRAISE mask.
>
> Â With the use of action mask only one efficient matching function
> Â is enough. Â Removed other specific versions of matching functions.
>
> Changelog:
> - change 'owner' to 'fowner' to conform to the new LSM conditions posted by
> Â Roberto Sassu.
> - fix calls to ima_log_string()
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> ---
>  Documentation/ABI/testing/ima_policy  |  25 +++++-
>  Documentation/kernel-parameters.txt  |   4 +
> Â security/integrity/ima/ima_appraise.c | Â Â 5 +-
>  security/integrity/ima/ima_policy.c  |  149 ++++++++++++++++++++++++---------
> Â 4 files changed, 139 insertions(+), 44 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 6cd6dae..dcff822 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -12,11 +12,14 @@ Description:
> Â Â Â Â Â Â Â Â then closing the file. Â The new policy takes effect after
> Â Â Â Â Â Â Â Â the file ima/policy is closed.
>
> + Â Â Â Â Â Â Â IMA appraisal, if configured, uses these file measurements
> + Â Â Â Â Â Â Â for local measurement appraisal.
> +
> Â Â Â Â Â Â Â Â rule format: action [condition ...]
>
> - Â Â Â Â Â Â Â action: measure | dont_measure
> + Â Â Â Â Â Â Â action: measure | dont_measure | appraise | dont_appraise
> Â Â Â Â Â Â Â Â condition:= base | lsm
> - Â Â Â Â Â Â Â Â Â Â Â base: Â [[func=] [mask=] [fsmagic=] [uid=]]
> + Â Â Â Â Â Â Â Â Â Â Â base: Â [[func=] [mask=] [fsmagic=] [uid=] [fowner]]
> Â Â Â Â Â Â Â Â Â Â Â Â lsm: Â Â [[subj_user=] [subj_role=] [subj_type=]
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â [obj_user=] [obj_role=] [obj_type=]]
>
> @@ -24,36 +27,50 @@ Description:
> Â Â Â Â Â Â Â Â Â Â Â Â mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
> Â Â Â Â Â Â Â Â Â Â Â Â fsmagic:= hex value
> Â Â Â Â Â Â Â Â Â Â Â Â uid:= decimal value
> + Â Â Â Â Â Â Â Â Â Â Â fowner:=decimal value
> Â Â Â Â Â Â Â Â lsm: Â Â are LSM specific
>
> Â Â Â Â Â Â Â Â default policy:
> Â Â Â Â Â Â Â Â Â Â Â Â # PROC_SUPER_MAGIC
> Â Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0x9fa0
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise fsmagic=0x9fa0
> Â Â Â Â Â Â Â Â Â Â Â Â # SYSFS_MAGIC
> Â Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0x62656572
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise fsmagic=0x62656572
> Â Â Â Â Â Â Â Â Â Â Â Â # DEBUGFS_MAGIC
> Â Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0x64626720
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise fsmagic=0x64626720
> Â Â Â Â Â Â Â Â Â Â Â Â # TMPFS_MAGIC
> Â Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0x01021994
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise fsmagic=0x01021994
> + Â Â Â Â Â Â Â Â Â Â Â # RAMFS_MAGIC
> + Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0x858458f6
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise fsmagic=0x858458f6
> Â Â Â Â Â Â Â Â Â Â Â Â # SECURITYFS_MAGIC
> Â Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0x73636673
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise fsmagic=0x73636673
>
> Â Â Â Â Â Â Â Â Â Â Â Â measure func=BPRM_CHECK
> Â Â Â Â Â Â Â Â Â Â Â Â measure func=FILE_MMAP mask=MAY_EXEC
> Â Â Â Â Â Â Â Â Â Â Â Â measure func=FILE_CHECK mask=MAY_READ uid=0
> + Â Â Â Â Â Â Â Â Â Â Â appraise fowner=0
>
> Â Â Â Â Â Â Â Â The default policy measures all executables in bprm_check,
> Â Â Â Â Â Â Â Â all files mmapped executable in file_mmap, and all files
> - Â Â Â Â Â Â Â open for read by root in do_filp_open.
> + Â Â Â Â Â Â Â open for read by root in do_filp_open. Â The default appraisal
> + Â Â Â Â Â Â Â policy appraises all files owned by root.
>
> Â Â Â Â Â Â Â Â Examples of LSM specific definitions:
>
> Â Â Â Â Â Â Â Â SELinux:
> Â Â Â Â Â Â Â Â Â Â Â Â # SELINUX_MAGIC
> - Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0xF97CFF8C
> + Â Â Â Â Â Â Â Â Â Â Â dont_measure fsmagic=0xf97cff8c
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise fsmagic=0xf97cff8c
>
> Â Â Â Â Â Â Â Â Â Â Â Â dont_measure obj_type=var_log_t
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise obj_type=var_log_t
> Â Â Â Â Â Â Â Â Â Â Â Â dont_measure obj_type=auditd_log_t
> + Â Â Â Â Â Â Â Â Â Â Â dont_appraise obj_type=auditd_log_t
> Â Â Â Â Â Â Â Â Â Â Â Â measure subj_user=system_u func=FILE_CHECK mask=MAY_READ
> Â Â Â Â Â Â Â Â Â Â Â Â measure subj_role=system_r func=FILE_CHECK mask=MAY_READ
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index a86765d..6c00491 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1008,6 +1008,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> Â Â Â Â Â Â Â Â Â Â Â Â Format: { "off" | "enforce" | "fix" }
> Â Â Â Â Â Â Â Â Â Â Â Â default: "enforce"
>
> + Â Â Â ima_appraise_tcb [IMA]
> + Â Â Â Â Â Â Â Â Â Â Â The builtin appraise policy appraises all files
> + Â Â Â Â Â Â Â Â Â Â Â owned by uid=0.
> +
> Â Â Â Â ima_audit= Â Â Â [IMA]
> Â Â Â Â Â Â Â Â Â Â Â Â Format: { "0" | "1" }
> Â Â Â Â Â Â Â Â Â Â Â Â 0 -- integrity auditing messages. (Default)
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 4865f61..681cb6e 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -36,7 +36,10 @@ __setup("ima_appraise=", default_appraise_setup);
> Â */
> Â int ima_must_appraise(struct inode *inode, enum ima_hooks func, int mask)
> Â {
> - Â Â Â return 0;
> + Â Â Â if (!ima_appraise)
> + Â Â Â Â Â Â Â return 0;
> +
> + Â Â Â return ima_match_policy(inode, func, mask, IMA_APPRAISE);
> Â }
>
> Â static void ima_fix_xattr(struct dentry *dentry,
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8ee301c..238aa2b 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -24,6 +24,7 @@
> Â #define IMA_MASK Â Â Â 0x0002
> Â #define IMA_FSMAGIC Â Â 0x0004
> Â #define IMA_UID Â Â Â Â Â Â Â Â 0x0008
> +#define IMA_FOWNER Â Â 0x0010
>
> Â #define UNKNOWN Â Â Â Â Â Â Â Â Â Â Â Â 0
> Â #define MEASURE Â Â Â Â Â Â Â Â Â Â Â Â 1 Â Â Â /* same as IMA_MEASURE */
> @@ -38,7 +39,7 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
> Â Â Â Â LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE
> Â };
>
> -struct ima_measure_rule_entry {
> +struct ima_rule_entry {
> Â Â Â Â struct list_head list;
> Â Â Â Â int action;
> Â Â Â Â unsigned int flags;
> @@ -46,6 +47,7 @@ struct ima_measure_rule_entry {
> Â Â Â Â int mask;
> Â Â Â Â unsigned long fsmagic;
> Â Â Â Â uid_t uid;
> + Â Â Â uid_t fowner;
> Â Â Â Â struct {
> Â Â Â Â Â Â Â Â void *rule; Â Â /* LSM file metadata specific */
> Â Â Â Â Â Â Â Â int type; Â Â Â /* audit type */
> @@ -54,7 +56,7 @@ struct ima_measure_rule_entry {
>
> Â /*
> Â * Without LSM specific knowledge, the default policy can only be
> - * written in terms of .action, .func, .mask, .fsmagic, and .uid
> + * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
> Â */
>
> Â /*
> @@ -63,7 +65,7 @@ struct ima_measure_rule_entry {
> Â * normal users can easily run the machine out of memory simply building
> Â * and running executables.
> Â */
> -static struct ima_measure_rule_entry default_rules[] = {
> +static struct ima_rule_entry default_rules[] = {
> Â Â Â Â {.action = DONT_MEASURE,.fsmagic = PROC_SUPER_MAGIC,.flags = IMA_FSMAGIC},
> Â Â Â Â {.action = DONT_MEASURE,.fsmagic = SYSFS_MAGIC,.flags = IMA_FSMAGIC},
> Â Â Â Â {.action = DONT_MEASURE,.fsmagic = DEBUGFS_MAGIC,.flags = IMA_FSMAGIC},
> @@ -79,19 +81,39 @@ static struct ima_measure_rule_entry default_rules[] = {
> Â Â Â Â .flags = IMA_FUNC | IMA_MASK | IMA_UID},
> Â };
>
> -static LIST_HEAD(measure_default_rules);
> -static LIST_HEAD(measure_policy_rules);
> -static struct list_head *ima_measure;
> +static struct ima_rule_entry default_appraise_rules[] = {
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = PROC_SUPER_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = SYSFS_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = DEBUGFS_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = RAMFS_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = SECURITYFS_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = SELINUX_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = DONT_APPRAISE,.fsmagic = CGROUP_SUPER_MAGIC,.flags = IMA_FSMAGIC},
> + Â Â Â {.action = APPRAISE,.fowner = 0,.flags = IMA_FOWNER},
> +};
> +
> +static LIST_HEAD(ima_default_rules);
> +static LIST_HEAD(ima_policy_rules);
> +static struct list_head *ima_rules;
>
> -static DEFINE_MUTEX(ima_measure_mutex);
> +static DEFINE_MUTEX(ima_rules_mutex);
>
> Â static bool ima_use_tcb __initdata;
> -static int __init default_policy_setup(char *str)
> +static int __init default_measure_policy_setup(char *str)
> Â {
> Â Â Â Â ima_use_tcb = 1;
> Â Â Â Â return 1;
> Â }
> -__setup("ima_tcb", default_policy_setup);
> +__setup("ima_tcb", default_measure_policy_setup);
> +
> +static bool ima_use_appraise_tcb __initdata;
> +static int __init default_appraise_policy_setup(char *str)
> +{
> + Â Â Â ima_use_appraise_tcb = 1;
> + Â Â Â return 1;
> +}
> +__setup("ima_appraise_tcb", default_appraise_policy_setup);
>
> Â /**
> Â * ima_match_rules - determine whether an inode matches the measure rule.
> @@ -102,7 +124,7 @@ __setup("ima_tcb", default_policy_setup);
> Â *
> Â * Returns true on rule match, false on failure.
> Â */
> -static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> +static bool ima_match_rules(struct ima_rule_entry *rule,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct inode *inode, enum ima_hooks func, int mask)
> Â {
> Â Â Â Â struct task_struct *tsk = current;
> @@ -118,6 +140,8 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> Â Â Â Â Â Â Â Â return false;
> Â Â Â Â if ((rule->flags & IMA_UID) && rule->uid != cred->uid)
> Â Â Â Â Â Â Â Â return false;
> + Â Â Â if ((rule->flags & IMA_FOWNER) && rule->fowner != inode->i_uid)
> + Â Â Â Â Â Â Â return false;
> Â Â Â Â for (i = 0; i < MAX_LSM_RULES; i++) {
> Â Â Â Â Â Â Â Â int rc = 0;
> Â Â Â Â Â Â Â Â u32 osid, sid;
> @@ -170,10 +194,10 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
> Â int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
> Â Â Â Â Â Â Â Â Â Â int flags)
> Â {
> - Â Â Â struct ima_measure_rule_entry *entry;
> + Â Â Â struct ima_rule_entry *entry;
> Â Â Â Â int action = 0, actmask = flags | (flags << 1);
>
> - Â Â Â list_for_each_entry(entry, ima_measure, list) {
> + Â Â Â list_for_each_entry(entry, ima_rules, list) {
>
> Â Â Â Â Â Â Â Â if (!(entry->action & actmask))
> Â Â Â Â Â Â Â Â Â Â Â Â continue;
> @@ -194,22 +218,31 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
> Â /**
> Â * ima_init_policy - initialize the default measure rules.
> Â *
> - * ima_measure points to either the measure_default_rules or the
> - * the new measure_policy_rules.
> + * ima_rules points to either the ima_default_rules or the
> + * the new ima_policy_rules.
> Â */
> Â void __init ima_init_policy(void)
> Â {
> - Â Â Â int i, entries;
> + Â Â Â int i, measure_entries, appraise_entries;
>
> Â Â Â Â /* if !ima_use_tcb set entries = 0 so we load NO default rules */
> - Â Â Â if (ima_use_tcb)
> - Â Â Â Â Â Â Â entries = ARRAY_SIZE(default_rules);
> - Â Â Â else
> - Â Â Â Â Â Â Â entries = 0;
> -
> - Â Â Â for (i = 0; i < entries; i++)
> - Â Â Â Â Â Â Â list_add_tail(&default_rules[i].list, &measure_default_rules);
> - Â Â Â ima_measure = &measure_default_rules;
> + Â Â Â measure_entries = ima_use_tcb ? ARRAY_SIZE(default_rules) : 0;
> + Â Â Â appraise_entries = ima_use_appraise_tcb ?
> + Â Â Â Â Â Â Â Â Â Â Â Â ARRAY_SIZE(default_appraise_rules) : 0;
> +
> + Â Â Â for (i = 0; i < measure_entries + appraise_entries; i++) {
> + Â Â Â Â Â Â Â if (i < measure_entries)
> + Â Â Â Â Â Â Â Â Â Â Â list_add_tail(&default_rules[i].list,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &ima_default_rules);
> + Â Â Â Â Â Â Â else {
> + Â Â Â Â Â Â Â Â Â Â Â int j = i - measure_entries;
> +
> + Â Â Â Â Â Â Â Â Â Â Â list_add_tail(&default_appraise_rules[j].list,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &ima_default_rules);
> + Â Â Â Â Â Â Â }
> + Â Â Â }
> +
> + Â Â Â ima_rules = &ima_default_rules;
> Â }
>
> Â /**
> @@ -226,8 +259,8 @@ void ima_update_policy(void)
> Â Â Â Â int result = 1;
> Â Â Â Â int audit_info = 0;
>
> - Â Â Â if (ima_measure == &measure_default_rules) {
> - Â Â Â Â Â Â Â ima_measure = &measure_policy_rules;
> + Â Â Â if (ima_rules == &ima_default_rules) {
> + Â Â Â Â Â Â Â ima_rules = &ima_policy_rules;
> Â Â Â Â Â Â Â Â cause = "complete";
> Â Â Â Â Â Â Â Â result = 0;
> Â Â Â Â }
> @@ -238,14 +271,17 @@ void ima_update_policy(void)
> Â enum {
> Â Â Â Â Opt_err = -1,
> Â Â Â Â Opt_measure = 1, Opt_dont_measure,
> + Â Â Â Opt_appraise, Opt_dont_appraise,
> Â Â Â Â Opt_obj_user, Opt_obj_role, Opt_obj_type,
> Â Â Â Â Opt_subj_user, Opt_subj_role, Opt_subj_type,
> - Â Â Â Opt_func, Opt_mask, Opt_fsmagic, Opt_uid
> + Â Â Â Opt_func, Opt_mask, Opt_fsmagic, Opt_uid, Opt_fowner
> Â };
>
> Â static match_table_t policy_tokens = {
> Â Â Â Â {Opt_measure, "measure"},
> Â Â Â Â {Opt_dont_measure, "dont_measure"},
> + Â Â Â {Opt_appraise, "appraise"},
> + Â Â Â {Opt_dont_appraise, "dont_appraise"},
> Â Â Â Â {Opt_obj_user, "obj_user=%s"},
> Â Â Â Â {Opt_obj_role, "obj_role=%s"},
> Â Â Â Â {Opt_obj_type, "obj_type=%s"},
> @@ -256,10 +292,11 @@ static match_table_t policy_tokens = {
> Â Â Â Â {Opt_mask, "mask=%s"},
> Â Â Â Â {Opt_fsmagic, "fsmagic=%s"},
> Â Â Â Â {Opt_uid, "uid=%s"},
> + Â Â Â {Opt_fowner, "fowner=%s"},
> Â Â Â Â {Opt_err, NULL}
> Â };
>
> -static int ima_lsm_rule_init(struct ima_measure_rule_entry *entry,
> +static int ima_lsm_rule_init(struct ima_rule_entry *entry,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â char *args, int lsm_rule, int audit_type)
> Â {
> Â Â Â Â int result;
> @@ -283,7 +320,7 @@ static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
> Â Â Â Â audit_log_format(ab, " ");
> Â }
>
> -static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
> +static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> Â {
> Â Â Â Â struct audit_buffer *ab;
> Â Â Â Â char *p;
> @@ -292,6 +329,7 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
> Â Â Â Â ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
>
> Â Â Â Â entry->uid = -1;
> + Â Â Â entry->fowner = -1;
> Â Â Â Â entry->action = UNKNOWN;
> Â Â Â Â while ((p = strsep(&rule, " \t")) != NULL) {
> Â Â Â Â Â Â Â Â substring_t args[MAX_OPT_ARGS];
> @@ -320,11 +358,27 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
>
> Â Â Â Â Â Â Â Â Â Â Â Â entry->action = DONT_MEASURE;
> Â Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â Â Â Â Â case Opt_appraise:
> + Â Â Â Â Â Â Â Â Â Â Â ima_log_string(ab, "action", "appraise");
> +
> + Â Â Â Â Â Â Â Â Â Â Â if (entry->action != UNKNOWN)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â result = -EINVAL;
> +
> + Â Â Â Â Â Â Â Â Â Â Â entry->action = APPRAISE;
> + Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â Â Â Â Â case Opt_dont_appraise:
> + Â Â Â Â Â Â Â Â Â Â Â ima_log_string(ab, "action", "dont_appraise");
> +
> + Â Â Â Â Â Â Â Â Â Â Â if (entry->action != UNKNOWN)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â result = -EINVAL;
> +
> + Â Â Â Â Â Â Â Â Â Â Â entry->action = DONT_APPRAISE;
> + Â Â Â Â Â Â Â Â Â Â Â break;
> Â Â Â Â Â Â Â Â case Opt_func:
> Â Â Â Â Â Â Â Â Â Â Â Â ima_log_string(ab, "func", args[0].from);
>
> Â Â Â Â Â Â Â Â Â Â Â Â if (entry->func)
> -                result  = -EINVAL;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â result = -EINVAL;
>
> Â Â Â Â Â Â Â Â Â Â Â Â if (strcmp(args[0].from, "FILE_CHECK") == 0)
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â entry->func = FILE_CHECK;
> @@ -389,6 +443,23 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â entry->flags |= IMA_UID;
> Â Â Â Â Â Â Â Â Â Â Â Â }
> Â Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â Â Â Â Â case Opt_fowner:
> + Â Â Â Â Â Â Â Â Â Â Â ima_log_string(ab, "fowner", args[0].from);
> +
> + Â Â Â Â Â Â Â Â Â Â Â if (entry->fowner != -1) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â result = -EINVAL;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â Â Â Â Â Â Â Â Â }
> +
> + Â Â Â Â Â Â Â Â Â Â Â result = strict_strtoul(args[0].from, 10, &lnum);
> + Â Â Â Â Â Â Â Â Â Â Â if (!result) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â entry->fowner = (uid_t) lnum;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (entry->fowner != lnum)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â result = -EINVAL;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â else
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â entry->flags |= IMA_FOWNER;
> + Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â Â Â Â Â break;
> Â Â Â Â Â Â Â Â case Opt_obj_user:
> Â Â Â Â Â Â Â Â Â Â Â Â ima_log_string(ab, "obj_user", args[0].from);
> Â Â Â Â Â Â Â Â Â Â Â Â result = ima_lsm_rule_init(entry, args[0].from,
> @@ -440,7 +511,7 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
> Â }
>
> Â /**
> - * ima_parse_add_rule - add a rule to measure_policy_rules
> + * ima_parse_add_rule - add a rule to ima_policy_rules
> Â * @rule - ima measurement policy rule
> Â *
> Â * Uses a mutex to protect the policy list from multiple concurrent writers.
> @@ -450,12 +521,12 @@ ssize_t ima_parse_add_rule(char *rule)
> Â {
> Â Â Â Â const char *op = "update_policy";
> Â Â Â Â char *p;
> - Â Â Â struct ima_measure_rule_entry *entry;
> + Â Â Â struct ima_rule_entry *entry;
> Â Â Â Â ssize_t result, len;
> Â Â Â Â int audit_info = 0;
>
> Â Â Â Â /* Prevent installed policy from changing */
> - Â Â Â if (ima_measure != &measure_default_rules) {
> + Â Â Â if (ima_rules != &ima_default_rules) {
> Â Â Â Â Â Â Â Â integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â NULL, op, "already exists",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â -EACCES, audit_info);
> @@ -488,9 +559,9 @@ ssize_t ima_parse_add_rule(char *rule)
> Â Â Â Â Â Â Â Â return result;
> Â Â Â Â }
>
> - Â Â Â mutex_lock(&ima_measure_mutex);
> - Â Â Â list_add_tail(&entry->list, &measure_policy_rules);
> - Â Â Â mutex_unlock(&ima_measure_mutex);
> + Â Â Â mutex_lock(&ima_rules_mutex);
> + Â Â Â list_add_tail(&entry->list, &ima_policy_rules);
> + Â Â Â mutex_unlock(&ima_rules_mutex);
>
> Â Â Â Â return len;
> Â }
> @@ -498,12 +569,12 @@ ssize_t ima_parse_add_rule(char *rule)
> Â /* ima_delete_rules called to cleanup invalid policy */
> Â void ima_delete_rules(void)
> Â {
> - Â Â Â struct ima_measure_rule_entry *entry, *tmp;
> + Â Â Â struct ima_rule_entry *entry, *tmp;
>
> - Â Â Â mutex_lock(&ima_measure_mutex);
> - Â Â Â list_for_each_entry_safe(entry, tmp, &measure_policy_rules, list) {
> + Â Â Â mutex_lock(&ima_rules_mutex);
> + Â Â Â list_for_each_entry_safe(entry, tmp, &ima_policy_rules, list) {
> Â Â Â Â Â Â Â Â list_del(&entry->list);
> Â Â Â Â Â Â Â Â kfree(entry);
> Â Â Â Â }
> - Â Â Â mutex_unlock(&ima_measure_mutex);
> + Â Â Â mutex_unlock(&ima_rules_mutex);
> Â }
> --
> 1.7.6.5
>
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 06/12] ima: allocating iint improvements
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (4 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 05/12] ima: add appraise action keywords and default rules Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 07/12] ima: replace iint spinlock with rwlock/read_lock Mimi Zohar
` (5 subsequent siblings)
11 siblings, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Dmitry Kasatkin, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
From: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
With IMA-appraisal's removal of the iint mutex and taking the i_mutex
instead, allocating the iint becomes a lot simplier, as we don't need
to be concerned with two processes racing to allocate the iint. This
patch cleans up and improves performance for allocating the iint.
- removed redundant double i_mutex locking
- combined iint allocation with tree search
Changelog v2:
- removed the rwlock/read_lock changes from this patch
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
include/linux/integrity.h | 7 +++--
security/integrity/iint.c | 45 +++++++++++++++---------------------
security/integrity/ima/ima_main.c | 13 +++-------
3 files changed, 27 insertions(+), 38 deletions(-)
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index a0c4125..66c5fe9 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -22,13 +22,14 @@ enum integrity_status {
/* List of EVM protected security xattrs */
#ifdef CONFIG_INTEGRITY
-extern int integrity_inode_alloc(struct inode *inode);
+extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
extern void integrity_inode_free(struct inode *inode);
#else
-static inline int integrity_inode_alloc(struct inode *inode)
+static inline struct integrity_iint_cache *
+ integrity_inode_get(struct inode *inode)
{
- return 0;
+ return NULL;
}
static inline void integrity_inode_free(struct inode *inode)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index e600986..c91a436 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -80,24 +80,26 @@ static void iint_free(struct integrity_iint_cache *iint)
}
/**
- * integrity_inode_alloc - allocate an iint associated with an inode
+ * integrity_inode_get - find or allocate an iint associated with an inode
* @inode: pointer to the inode
+ * @return: allocated iint
+ *
+ * Caller must lock i_mutex
*/
-int integrity_inode_alloc(struct inode *inode)
+struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
{
struct rb_node **p;
- struct rb_node *new_node, *parent = NULL;
- struct integrity_iint_cache *new_iint, *test_iint;
- int rc;
+ struct rb_node *node, *parent = NULL;
+ struct integrity_iint_cache *iint, *test_iint;
- new_iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
- if (!new_iint)
- return -ENOMEM;
+ iint = integrity_iint_find(inode);
+ if (iint)
+ return iint;
- new_iint->inode = inode;
- new_node = &new_iint->rb_node;
+ iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
+ if (!iint)
+ return NULL;
- mutex_lock(&inode->i_mutex); /* i_flags */
spin_lock(&integrity_iint_lock);
p = &integrity_iint_tree.rb_node;
@@ -105,29 +107,20 @@ int integrity_inode_alloc(struct inode *inode)
parent = *p;
test_iint = rb_entry(parent, struct integrity_iint_cache,
rb_node);
- rc = -EEXIST;
if (inode < test_iint->inode)
p = &(*p)->rb_left;
- else if (inode > test_iint->inode)
- p = &(*p)->rb_right;
else
- goto out_err;
+ p = &(*p)->rb_right;
}
+ iint->inode = inode;
+ node = &iint->rb_node;
inode->i_flags |= S_IMA;
- rb_link_node(new_node, parent, p);
- rb_insert_color(new_node, &integrity_iint_tree);
+ rb_link_node(node, parent, p);
+ rb_insert_color(node, &integrity_iint_tree);
spin_unlock(&integrity_iint_lock);
- mutex_unlock(&inode->i_mutex); /* i_flags */
-
- return 0;
-out_err:
- spin_unlock(&integrity_iint_lock);
- mutex_unlock(&inode->i_mutex); /* i_flags */
- iint_free(new_iint);
-
- return rc;
+ return iint;
}
/**
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index fba2f7b..9fe0597 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -145,19 +145,14 @@ static int process_measurement(struct file *file, const unsigned char *filename,
if (!action)
return 0;
-retry:
- iint = integrity_iint_find(inode);
- if (!iint) {
- rc = integrity_inode_alloc(inode);
- if (!rc || rc == -EEXIST)
- goto retry;
- return rc;
- }
-
must_appraise = action & IMA_APPRAISE;
mutex_lock(&inode->i_mutex);
+ iint = integrity_inode_get(inode);
+ if (!iint)
+ goto out;
+
/* Determine if already appraised/measured based on bitmask
* (IMA_MEASURE, IMA_MEASURED, IMA_APPRAISE, IMA_APPRAISED) */
iint->flags |= action;
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 07/12] ima: replace iint spinlock with rwlock/read_lock
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (5 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 06/12] ima: allocating iint improvements Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 08/12] ima: add inode_post_setattr call Mimi Zohar
` (4 subsequent siblings)
11 siblings, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Mimi Zohar
For performance, replace the iint spinlock with rwlock/read_lock.
Eric Paris questioned this change, from spinlocks to rwlocks, saying
"rwlocks have been shown to actually be slower on multi processor
systems in a number of cases due to the cache line bouncing required."
Based on performance measurements compiling the kernel on a cold
boot with multiple jobs with/without this patch, Dmitry Kasatkin
and I found that rwlocks performed better than spinlocks, but very
insignificantly. For example with total compilation time around 6
minutes, with rwlocks time was 1 - 3 seconds shorter... but always
like that.
Changelog v2:
- new patch taken from the 'allocating iint improvements' patch
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
security/integrity/iint.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c91a436..d82a5a1 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -22,7 +22,7 @@
#include "integrity.h"
static struct rb_root integrity_iint_tree = RB_ROOT;
-static DEFINE_SPINLOCK(integrity_iint_lock);
+static DEFINE_RWLOCK(integrity_iint_lock);
static struct kmem_cache *iint_cache __read_mostly;
int iint_initialized;
@@ -35,8 +35,6 @@ static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode)
struct integrity_iint_cache *iint;
struct rb_node *n = integrity_iint_tree.rb_node;
- assert_spin_locked(&integrity_iint_lock);
-
while (n) {
iint = rb_entry(n, struct integrity_iint_cache, rb_node);
@@ -63,9 +61,9 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
if (!IS_IMA(inode))
return NULL;
- spin_lock(&integrity_iint_lock);
+ read_lock(&integrity_iint_lock);
iint = __integrity_iint_find(inode);
- spin_unlock(&integrity_iint_lock);
+ read_unlock(&integrity_iint_lock);
return iint;
}
@@ -100,7 +98,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
if (!iint)
return NULL;
- spin_lock(&integrity_iint_lock);
+ write_lock(&integrity_iint_lock);
p = &integrity_iint_tree.rb_node;
while (*p) {
@@ -119,7 +117,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
rb_link_node(node, parent, p);
rb_insert_color(node, &integrity_iint_tree);
- spin_unlock(&integrity_iint_lock);
+ write_unlock(&integrity_iint_lock);
return iint;
}
@@ -136,10 +134,10 @@ void integrity_inode_free(struct inode *inode)
if (!IS_IMA(inode))
return;
- spin_lock(&integrity_iint_lock);
+ write_lock(&integrity_iint_lock);
iint = __integrity_iint_find(inode);
rb_erase(&iint->rb_node, &integrity_iint_tree);
- spin_unlock(&integrity_iint_lock);
+ write_unlock(&integrity_iint_lock);
iint_free(iint);
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 08/12] ima: add inode_post_setattr call
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (6 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 07/12] ima: replace iint spinlock with rwlock/read_lock Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-22 14:21 ` Kasatkin, Dmitry
2012-03-21 18:54 ` [PATCH v3 09/12] ima: add ima_inode_setxattr/removexattr function and calls Mimi Zohar
` (3 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Mimi Zohar
Changing an inode's metadata may result in our not needing to appraise
the file. In such cases, we must remove 'security.ima'.
Changelog v1:
- use ima_inode_post_setattr() stub function, if IMA_APPRAISE not configured
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
fs/attr.c | 2 ++
include/linux/ima.h | 10 ++++++++++
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index d9bd7fc..f496bac 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -14,6 +14,7 @@
#include <linux/fcntl.h>
#include <linux/security.h>
#include <linux/evm.h>
+#include <linux/ima.h>
/**
* inode_change_ok - check if attribute changes to an inode are allowed
@@ -245,6 +246,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
if (!error) {
fsnotify_change(dentry, ia_valid);
+ ima_inode_post_setattr(dentry);
evm_inode_post_setattr(dentry, ia_valid);
}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6ac8e50..e2bfbb1 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -39,5 +39,15 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
{
return 0;
}
+
#endif /* CONFIG_IMA_H */
+
+#ifdef CONFIG_IMA_APPRAISE
+extern void ima_inode_post_setattr(struct dentry *dentry);
+#else
+static inline void ima_inode_post_setattr(struct dentry *dentry)
+{
+ return;
+}
+#endif /* CONFIG_IMA_APPRAISE_H */
#endif /* _LINUX_IMA_H */
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 08/12] ima: add inode_post_setattr call
2012-03-21 18:54 ` [PATCH v3 08/12] ima: add inode_post_setattr call Mimi Zohar
@ 2012-03-22 14:21 ` Kasatkin, Dmitry
0 siblings, 0 replies; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 14:21 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
On Wed, Mar 21, 2012 at 8:54 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Changing an inode's metadata may result in our not needing to appraise
> the file. In such cases, we must remove 'security.ima'.
>
> Changelog v1:
> - use ima_inode_post_setattr() stub function, if IMA_APPRAISE not configured
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> Acked-by: Serge Hallyn <serge.hallyn@ubuntu.com>
Acked-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> ---
> fs/attr.c | 2 ++
> include/linux/ima.h | 10 ++++++++++
> 2 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index d9bd7fc..f496bac 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -14,6 +14,7 @@
> #include <linux/fcntl.h>
> #include <linux/security.h>
> #include <linux/evm.h>
> +#include <linux/ima.h>
>
> /**
> * inode_change_ok - check if attribute changes to an inode are allowed
> @@ -245,6 +246,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
>
> if (!error) {
> fsnotify_change(dentry, ia_valid);
> + ima_inode_post_setattr(dentry);
> evm_inode_post_setattr(dentry, ia_valid);
> }
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 6ac8e50..e2bfbb1 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -39,5 +39,15 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
> {
> return 0;
> }
> +
> #endif /* CONFIG_IMA_H */
> +
> +#ifdef CONFIG_IMA_APPRAISE
> +extern void ima_inode_post_setattr(struct dentry *dentry);
> +#else
> +static inline void ima_inode_post_setattr(struct dentry *dentry)
> +{
> + return;
> +}
> +#endif /* CONFIG_IMA_APPRAISE_H */
> #endif /* _LINUX_IMA_H */
> --
> 1.7.6.5
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 09/12] ima: add ima_inode_setxattr/removexattr function and calls
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (7 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 08/12] ima: add inode_post_setattr call Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-22 14:22 ` Kasatkin, Dmitry
2012-03-21 18:54 ` [PATCH v3 10/12] ima: defer calling __fput() Mimi Zohar
` (2 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Mimi Zohar
Based on xattr_permission comments, the restriction to modify 'security'
xattr is left up to the underlying fs or lsm. Ensure that not just anyone
can modify or remove 'security.ima'.
Changelog v1:
- Unless IMA-APPRAISE is configured, use stub ima_inode_removexattr()/setxattr()
functions. (Moved ima_inode_removexattr()/setxattr() to ima_appraise.c)
Changelog:
- take i_mutex to fix locking (Dmitry Kasatkin)
- ima_reset_appraise_flags should only be called when modifying or
removing the 'security.ima' xattr. Requires CAP_SYS_ADMIN privilege.
(Incorporated fix from Roberto Sassu)
- Even if allowed to update security.ima, reset the appraisal flags,
forcing re-appraisal.
- Replace CAP_MAC_ADMIN with CAP_SYS_ADMIN
- static inline ima_inode_setxattr()/ima_inode_removexattr() stubs
- ima_protect_xattr should be static
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
include/linux/ima.h | 17 ++++++++++
security/integrity/ima/ima_appraise.c | 57 +++++++++++++++++++++++++++++++++
security/security.c | 6 +++
3 files changed, 80 insertions(+), 0 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index e2bfbb1..2c7223d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -44,10 +44,27 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
#ifdef CONFIG_IMA_APPRAISE
extern void ima_inode_post_setattr(struct dentry *dentry);
+extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
+ const void *xattr_value, size_t xattr_value_len);
+extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
#else
static inline void ima_inode_post_setattr(struct dentry *dentry)
{
return;
}
+
+static inline int ima_inode_setxattr(struct dentry *dentry,
+ const char *xattr_name,
+ const void *xattr_value,
+ size_t xattr_value_len)
+{
+ return 0;
+}
+
+static inline int ima_inode_removexattr(struct dentry *dentry,
+ const char *xattr_name)
+{
+ return 0;
+}
#endif /* CONFIG_IMA_APPRAISE_H */
#endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 681cb6e..becc7e0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -169,3 +169,60 @@ void ima_inode_post_setattr(struct dentry *dentry)
rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
return;
}
+
+/*
+ * ima_protect_xattr - protect 'security.ima'
+ *
+ * Ensure that not just anyone can modify or remove 'security.ima'.
+ */
+static int ima_protect_xattr(struct dentry *dentry, const char *xattr_name,
+ const void *xattr_value, size_t xattr_value_len)
+{
+ if (strcmp(xattr_name, XATTR_NAME_IMA) == 0) {
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ return 1;
+ }
+ return 0;
+}
+
+static void ima_reset_appraise_flags(struct inode *inode)
+{
+ struct integrity_iint_cache *iint;
+
+ if (!ima_initialized || !ima_appraise || !S_ISREG(inode->i_mode))
+ return;
+
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return;
+
+ iint->flags &= ~(IMA_COLLECTED | IMA_APPRAISED | IMA_MEASURED);
+ return;
+}
+
+int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
+ const void *xattr_value, size_t xattr_value_len)
+{
+ int result;
+
+ result = ima_protect_xattr(dentry, xattr_name, xattr_value,
+ xattr_value_len);
+ if (result == 1) {
+ ima_reset_appraise_flags(dentry->d_inode);
+ result = 0;
+ }
+ return result;
+}
+
+int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
+{
+ int result;
+
+ result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
+ if (result == 1) {
+ ima_reset_appraise_flags(dentry->d_inode);
+ result = 0;
+ }
+ return result;
+}
diff --git a/security/security.c b/security/security.c
index e50bbf4..7bb127e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -557,6 +557,9 @@ int security_inode_setxattr(struct dentry *dentry, const char *name,
ret = security_ops->inode_setxattr(dentry, name, value, size, flags);
if (ret)
return ret;
+ ret = ima_inode_setxattr(dentry, name, value, size);
+ if (ret)
+ return ret;
return evm_inode_setxattr(dentry, name, value, size);
}
@@ -592,6 +595,9 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
ret = security_ops->inode_removexattr(dentry, name);
if (ret)
return ret;
+ ret = ima_inode_removexattr(dentry, name);
+ if (ret)
+ return ret;
return evm_inode_removexattr(dentry, name);
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 09/12] ima: add ima_inode_setxattr/removexattr function and calls
2012-03-21 18:54 ` [PATCH v3 09/12] ima: add ima_inode_setxattr/removexattr function and calls Mimi Zohar
@ 2012-03-22 14:22 ` Kasatkin, Dmitry
0 siblings, 0 replies; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 14:22 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
On Wed, Mar 21, 2012 at 8:54 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Based on xattr_permission comments, the restriction to modify 'security'
> xattr is left up to the underlying fs or lsm. Ensure that not just anyone
> can modify or remove 'security.ima'.
>
> Changelog v1:
> - Unless IMA-APPRAISE is configured, use stub ima_inode_removexattr()/setxattr()
> functions. (Moved ima_inode_removexattr()/setxattr() to ima_appraise.c)
>
> Changelog:
> - take i_mutex to fix locking (Dmitry Kasatkin)
> - ima_reset_appraise_flags should only be called when modifying or
> removing the 'security.ima' xattr. Requires CAP_SYS_ADMIN privilege.
> (Incorporated fix from Roberto Sassu)
> - Even if allowed to update security.ima, reset the appraisal flags,
> forcing re-appraisal.
> - Replace CAP_MAC_ADMIN with CAP_SYS_ADMIN
> - static inline ima_inode_setxattr()/ima_inode_removexattr() stubs
> - ima_protect_xattr should be static
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> ---
> include/linux/ima.h | 17 ++++++++++
> security/integrity/ima/ima_appraise.c | 57 +++++++++++++++++++++++++++++++++
> security/security.c | 6 +++
> 3 files changed, 80 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index e2bfbb1..2c7223d 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -44,10 +44,27 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
>
> #ifdef CONFIG_IMA_APPRAISE
> extern void ima_inode_post_setattr(struct dentry *dentry);
> +extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> + const void *xattr_value, size_t xattr_value_len);
> +extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
> #else
> static inline void ima_inode_post_setattr(struct dentry *dentry)
> {
> return;
> }
> +
> +static inline int ima_inode_setxattr(struct dentry *dentry,
> + const char *xattr_name,
> + const void *xattr_value,
> + size_t xattr_value_len)
> +{
> + return 0;
> +}
> +
> +static inline int ima_inode_removexattr(struct dentry *dentry,
> + const char *xattr_name)
> +{
> + return 0;
> +}
> #endif /* CONFIG_IMA_APPRAISE_H */
> #endif /* _LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 681cb6e..becc7e0 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -169,3 +169,60 @@ void ima_inode_post_setattr(struct dentry *dentry)
> rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
> return;
> }
> +
> +/*
> + * ima_protect_xattr - protect 'security.ima'
> + *
> + * Ensure that not just anyone can modify or remove 'security.ima'.
> + */
> +static int ima_protect_xattr(struct dentry *dentry, const char *xattr_name,
> + const void *xattr_value, size_t xattr_value_len)
> +{
> + if (strcmp(xattr_name, XATTR_NAME_IMA) == 0) {
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + return 1;
> + }
> + return 0;
> +}
> +
> +static void ima_reset_appraise_flags(struct inode *inode)
> +{
> + struct integrity_iint_cache *iint;
> +
> + if (!ima_initialized || !ima_appraise || !S_ISREG(inode->i_mode))
> + return;
> +
> + iint = integrity_iint_find(inode);
> + if (!iint)
> + return;
> +
> + iint->flags &= ~(IMA_COLLECTED | IMA_APPRAISED | IMA_MEASURED);
> + return;
> +}
> +
> +int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> + const void *xattr_value, size_t xattr_value_len)
> +{
> + int result;
> +
> + result = ima_protect_xattr(dentry, xattr_name, xattr_value,
> + xattr_value_len);
> + if (result == 1) {
> + ima_reset_appraise_flags(dentry->d_inode);
> + result = 0;
> + }
> + return result;
> +}
> +
> +int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
> +{
> + int result;
> +
> + result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
> + if (result == 1) {
> + ima_reset_appraise_flags(dentry->d_inode);
> + result = 0;
> + }
> + return result;
> +}
> diff --git a/security/security.c b/security/security.c
> index e50bbf4..7bb127e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -557,6 +557,9 @@ int security_inode_setxattr(struct dentry *dentry, const char *name,
> ret = security_ops->inode_setxattr(dentry, name, value, size, flags);
> if (ret)
> return ret;
> + ret = ima_inode_setxattr(dentry, name, value, size);
> + if (ret)
> + return ret;
> return evm_inode_setxattr(dentry, name, value, size);
> }
>
> @@ -592,6 +595,9 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
> ret = security_ops->inode_removexattr(dentry, name);
> if (ret)
> return ret;
> + ret = ima_inode_removexattr(dentry, name);
> + if (ret)
> + return ret;
> return evm_inode_removexattr(dentry, name);
> }
>
> --
> 1.7.6.5
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 10/12] ima: defer calling __fput()
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (8 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 09/12] ima: add ima_inode_setxattr/removexattr function and calls Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-22 14:07 ` Kasatkin, Dmitry
2012-03-22 14:22 ` Al Viro
2012-03-21 18:54 ` [PATCH v3 11/12] ima: add support for different security.ima data types Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 12/12] ima: digital signature verification support Mimi Zohar
11 siblings, 2 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Matt Helsley, Mimi Zohar
If a file is closed before it is munmapped, __fput() is called with
the mmap_sem taken. With IMA-appraisal enabled, this results in an
mmap_sem/i_mutex lockdep. ima_defer_fput() increments the f_count
to defer __fput() being called until after the mmap_sem is released.
Changelog v3:
- use slab mempool (suggested by Dmitry Kasatkin)
- renamed a few functions based on Matt Helsley's review
- define ima_fput_mempool as static (fix sparse error)
- fix ima_defer_fput() stub definition
Changelog v2:
- based on discussions with: Tyler Hicks (use workqueues),
Dave Hansen (defer only IMA related files), and
Dmitry Kasatkin (add new IMA hook)
- use slab memory for allocating work
Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
include/linux/ima.h | 5 ++
mm/mmap.c | 1 +
security/integrity/ima/ima_appraise.c | 84 +++++++++++++++++++++++++++++++++
3 files changed, 90 insertions(+), 0 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 2c7223d..af8cb3b 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -47,6 +47,7 @@ extern void ima_inode_post_setattr(struct dentry *dentry);
extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
const void *xattr_value, size_t xattr_value_len);
extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
+extern void ima_defer_fput(struct file *file);
#else
static inline void ima_inode_post_setattr(struct dentry *dentry)
{
@@ -66,5 +67,9 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
{
return 0;
}
+static inline void ima_defer_fput(struct file *file)
+{
+ return;
+}
#endif /* CONFIG_IMA_APPRAISE_H */
#endif /* _LINUX_IMA_H */
diff --git a/mm/mmap.c b/mm/mmap.c
index 4c16a0a..c1accd4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -232,6 +232,7 @@ static struct vm_area_struct *remove_vma(struct vm_area_struct *vma)
if (vma->vm_ops && vma->vm_ops->close)
vma->vm_ops->close(vma);
if (vma->vm_file) {
+ ima_defer_fput(vma->vm_file); /* drop mmap_sem first */
fput(vma->vm_file);
if (vma->vm_flags & VM_EXECUTABLE)
removed_exe_file_vma(vma->vm_mm);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index becc7e0..a100a3c 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/file.h>
#include <linux/fs.h>
+#include <linux/mempool.h>
#include <linux/xattr.h>
#include <linux/magic.h>
#include <linux/ima.h>
@@ -226,3 +227,86 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
}
return result;
}
+
+struct ima_fput_req {
+ struct file *file;
+ struct work_struct work;
+};
+static struct kmem_cache *ima_fput_slab;
+static mempool_t *ima_fput_mempool;
+
+static void ima_deferred_fput(struct work_struct *work)
+{
+ struct ima_fput_req *deferred = container_of(work, struct ima_fput_req,
+ work);
+
+ fput(deferred->file);
+ mempool_free(deferred, ima_fput_mempool);
+}
+
+/**
+ * ima_defer_fput - defer calling __fput() for mmaped files
+ * @file: pointer to file structure to be freed
+ *
+ * The i_mutex is taken by ima_file_free(), called on __fput(), to
+ * update 'security.ima' to reflect the current file data hash. When
+ * a file is closed before it is munmapped, __fput() is called with
+ * the mmap_sem taken, resulting in an mmap_sem/i_mutex lockdep.
+ * This function defers calling __fput() until after the mmap_sem is
+ * released.
+ */
+void ima_defer_fput(struct file *file)
+{
+ struct dentry *dentry = file->f_path.dentry;
+ struct inode *inode = dentry->d_inode;
+ struct ima_fput_req *defer;
+ int rc;
+
+ if (!IS_IMA(inode))
+ return;
+
+ defer = mempool_alloc(ima_fput_mempool, GFP_NOIO);
+ if (!defer)
+ return;
+
+ get_file(file);
+
+ defer->file = file;
+ INIT_WORK(&defer->work, ima_deferred_fput);
+ rc = schedule_work(&defer->work);
+ if (rc == 0) {
+ pr_info("ima: schedule work failed\n");
+ mempool_free(defer, ima_fput_mempool);
+ fput_atomic(file);
+ }
+}
+
+static int __init ima_fput_mempool_create(void)
+{
+ ima_fput_slab = kmem_cache_create("ima_fput",
+ sizeof(struct ima_fput_req),
+ 0, 0, NULL);
+ if (!ima_fput_slab) {
+ pr_info("ima: failed kmem_cache_create\n");
+ goto err_out;
+ }
+
+ ima_fput_mempool = mempool_create_slab_pool(1, ima_fput_slab);
+ if (!ima_fput_mempool) {
+ pr_info("ima: failed mempool_create_slab_pool\n");
+ goto free_slab;
+ }
+ return 0;
+
+free_slab:
+ kmem_cache_destroy(ima_fput_slab);
+err_out:
+ return -ENOMEM;
+}
+
+static void __exit ima_fput_mempool_destroy(void)
+{
+ mempool_destroy(ima_fput_mempool);
+ kmem_cache_destroy(ima_fput_slab);
+}
+security_initcall(ima_fput_mempool_create);
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-21 18:54 ` [PATCH v3 10/12] ima: defer calling __fput() Mimi Zohar
@ 2012-03-22 14:07 ` Kasatkin, Dmitry
2012-03-22 14:22 ` Al Viro
1 sibling, 0 replies; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 14:07 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Matt Helsley, Mimi Zohar
Hello,
On Wed, Mar 21, 2012 at 8:54 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> If a file is closed before it is munmapped, __fput() is called with
> the mmap_sem taken. With IMA-appraisal enabled, this results in an
> mmap_sem/i_mutex lockdep. ima_defer_fput() increments the f_count
> to defer __fput() being called until after the mmap_sem is released.
>
Have you noticed any performance implications?
> Changelog v3:
> - use slab mempool (suggested by Dmitry Kasatkin)
> - renamed a few functions based on Matt Helsley's review
> - define ima_fput_mempool as static (fix sparse error)
> - fix ima_defer_fput() stub definition
>
> Changelog v2:
> - based on discussions with: Tyler Hicks (use workqueues),
> Dave Hansen (defer only IMA related files), and
> Dmitry Kasatkin (add new IMA hook)
> - use slab memory for allocating work
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> ---
> include/linux/ima.h | 5 ++
> mm/mmap.c | 1 +
> security/integrity/ima/ima_appraise.c | 84 +++++++++++++++++++++++++++++++++
> 3 files changed, 90 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 2c7223d..af8cb3b 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -47,6 +47,7 @@ extern void ima_inode_post_setattr(struct dentry *dentry);
> extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> const void *xattr_value, size_t xattr_value_len);
> extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
> +extern void ima_defer_fput(struct file *file);
> #else
> static inline void ima_inode_post_setattr(struct dentry *dentry)
> {
> @@ -66,5 +67,9 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
> {
> return 0;
> }
> +static inline void ima_defer_fput(struct file *file)
> +{
> + return;
> +}
> #endif /* CONFIG_IMA_APPRAISE_H */
> #endif /* _LINUX_IMA_H */
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 4c16a0a..c1accd4 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -232,6 +232,7 @@ static struct vm_area_struct *remove_vma(struct vm_area_struct *vma)
> if (vma->vm_ops && vma->vm_ops->close)
> vma->vm_ops->close(vma);
> if (vma->vm_file) {
> + ima_defer_fput(vma->vm_file); /* drop mmap_sem first */
> fput(vma->vm_file);
> if (vma->vm_flags & VM_EXECUTABLE)
> removed_exe_file_vma(vma->vm_mm);
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index becc7e0..a100a3c 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -11,6 +11,7 @@
> #include <linux/module.h>
> #include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/mempool.h>
> #include <linux/xattr.h>
> #include <linux/magic.h>
> #include <linux/ima.h>
> @@ -226,3 +227,86 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
> }
> return result;
> }
> +
> +struct ima_fput_req {
> + struct file *file;
> + struct work_struct work;
> +};
> +static struct kmem_cache *ima_fput_slab;
> +static mempool_t *ima_fput_mempool;
> +
> +static void ima_deferred_fput(struct work_struct *work)
> +{
> + struct ima_fput_req *deferred = container_of(work, struct ima_fput_req,
> + work);
> +
> + fput(deferred->file);
> + mempool_free(deferred, ima_fput_mempool);
> +}
> +
> +/**
> + * ima_defer_fput - defer calling __fput() for mmaped files
> + * @file: pointer to file structure to be freed
> + *
> + * The i_mutex is taken by ima_file_free(), called on __fput(), to
> + * update 'security.ima' to reflect the current file data hash. When
> + * a file is closed before it is munmapped, __fput() is called with
> + * the mmap_sem taken, resulting in an mmap_sem/i_mutex lockdep.
> + * This function defers calling __fput() until after the mmap_sem is
> + * released.
> + */
> +void ima_defer_fput(struct file *file)
> +{
> + struct dentry *dentry = file->f_path.dentry;
> + struct inode *inode = dentry->d_inode;
> + struct ima_fput_req *defer;
> + int rc;
> +
> + if (!IS_IMA(inode))
> + return;
> +
> + defer = mempool_alloc(ima_fput_mempool, GFP_NOIO);
> + if (!defer)
> + return;
> +
> + get_file(file);
> +
> + defer->file = file;
> + INIT_WORK(&defer->work, ima_deferred_fput);
> + rc = schedule_work(&defer->work);
> + if (rc == 0) {
> + pr_info("ima: schedule work failed\n");
> + mempool_free(defer, ima_fput_mempool);
> + fput_atomic(file);
> + }
> +}
> +
> +static int __init ima_fput_mempool_create(void)
> +{
> + ima_fput_slab = kmem_cache_create("ima_fput",
> + sizeof(struct ima_fput_req),
> + 0, 0, NULL);
> + if (!ima_fput_slab) {
> + pr_info("ima: failed kmem_cache_create\n");
> + goto err_out;
> + }
> +
> + ima_fput_mempool = mempool_create_slab_pool(1, ima_fput_slab);
> + if (!ima_fput_mempool) {
> + pr_info("ima: failed mempool_create_slab_pool\n");
> + goto free_slab;
> + }
> + return 0;
> +
> +free_slab:
> + kmem_cache_destroy(ima_fput_slab);
> +err_out:
> + return -ENOMEM;
> +}
> +
> +static void __exit ima_fput_mempool_destroy(void)
> +{
> + mempool_destroy(ima_fput_mempool);
> + kmem_cache_destroy(ima_fput_slab);
> +}
Is it really needed? I think it will never be called if not a module...
> +security_initcall(ima_fput_mempool_create);
> --
> 1.7.6.5
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-21 18:54 ` [PATCH v3 10/12] ima: defer calling __fput() Mimi Zohar
2012-03-22 14:07 ` Kasatkin, Dmitry
@ 2012-03-22 14:22 ` Al Viro
2012-03-22 14:53 ` Mimi Zohar
1 sibling, 1 reply; 26+ messages in thread
From: Al Viro @ 2012-03-22 14:22 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel,
David Safford, Dmitry Kasatkin, Matt Helsley, Mimi Zohar
On Wed, Mar 21, 2012 at 02:54:15PM -0400, Mimi Zohar wrote:
> If a file is closed before it is munmapped, __fput() is called with
> the mmap_sem taken. With IMA-appraisal enabled, this results in an
> mmap_sem/i_mutex lockdep. ima_defer_fput() increments the f_count
> to defer __fput() being called until after the mmap_sem is released.
NAK. It's far too heavy-weight for what's a very common path. Worse,
it causes different locking conditions on IMA and non-IMA kernels and makes
bugs harder to spot. Rejected with extreme prejudice; please, fix your
locking instead.
BTW, you've missed several other places in mm/* doing fput(), so it wouldn't
be enough to paper over your problem anyway.
Final fput() *can* happen under mmap_sem. Period.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-22 14:22 ` Al Viro
@ 2012-03-22 14:53 ` Mimi Zohar
2012-03-22 14:58 ` Kasatkin, Dmitry
2012-03-22 15:09 ` Al Viro
0 siblings, 2 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-22 14:53 UTC (permalink / raw)
To: Al Viro
Cc: linux-security-module, linux-kernel, linux-fsdevel,
David Safford, Dmitry Kasatkin, Matt Helsley, Mimi Zohar
On Thu, 2012-03-22 at 14:22 +0000, Al Viro wrote:
> On Wed, Mar 21, 2012 at 02:54:15PM -0400, Mimi Zohar wrote:
> > If a file is closed before it is munmapped, __fput() is called with
> > the mmap_sem taken. With IMA-appraisal enabled, this results in an
> > mmap_sem/i_mutex lockdep. ima_defer_fput() increments the f_count
> > to defer __fput() being called until after the mmap_sem is released.
>
> NAK. It's far too heavy-weight for what's a very common path. Worse,
> it causes different locking conditions on IMA and non-IMA kernels and makes
> bugs harder to spot. Rejected with extreme prejudice; please, fix your
> locking instead.
Al, thanks for replying! I realize you are very prejudiced against IMA,
but have you taken a look at IMA-appraisal? It probably would have been
better if we used another name in order to avoid this prejudice, but
unfortunately we didn't. :( IMA-appraisal is about enforcing file
integrity.
> BTW, you've missed several other places in mm/* doing fput(), so it wouldn't
> be enough to paper over your problem anyway.
>
> Final fput() *can* happen under mmap_sem. Period.
I think I got that loud and clear; otherwise we wouldn't have come up
with deferring the __fput(). We have a very real problem here - writing
extended attributes requires taking the i_mutex. I'm open to any
suggestions.
thanks,
Mimi
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-22 14:53 ` Mimi Zohar
@ 2012-03-22 14:58 ` Kasatkin, Dmitry
2012-03-22 15:09 ` Al Viro
1 sibling, 0 replies; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 14:58 UTC (permalink / raw)
To: Mimi Zohar
Cc: Al Viro, linux-security-module, linux-kernel, linux-fsdevel,
David Safford, Matt Helsley, Mimi Zohar
On Thu, Mar 22, 2012 at 4:53 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Thu, 2012-03-22 at 14:22 +0000, Al Viro wrote:
>> On Wed, Mar 21, 2012 at 02:54:15PM -0400, Mimi Zohar wrote:
>> > If a file is closed before it is munmapped, __fput() is called with
>> > the mmap_sem taken. With IMA-appraisal enabled, this results in an
>> > mmap_sem/i_mutex lockdep. ima_defer_fput() increments the f_count
>> > to defer __fput() being called until after the mmap_sem is released.
>>
>> NAK. It's far too heavy-weight for what's a very common path. Worse,
>> it causes different locking conditions on IMA and non-IMA kernels and makes
>> bugs harder to spot. Rejected with extreme prejudice; please, fix your
>> locking instead.
>
> Al, thanks for replying! I realize you are very prejudiced against IMA,
> but have you taken a look at IMA-appraisal? It probably would have been
> better if we used another name in order to avoid this prejudice, but
> unfortunately we didn't. :( IMA-appraisal is about enforcing file
> integrity.
>
>> BTW, you've missed several other places in mm/* doing fput(), so it wouldn't
>> be enough to paper over your problem anyway.
>>
In current solution, deferred call can be added directly to __fput and checking
if mmap_sem is locked.
>> Final fput() *can* happen under mmap_sem. Period.
>
> I think I got that loud and clear; otherwise we wouldn't have come up
> with deferring the __fput(). We have a very real problem here - writing
> extended attributes requires taking the i_mutex. I'm open to any
> suggestions.
>
> thanks,
>
> Mimi
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-22 14:53 ` Mimi Zohar
2012-03-22 14:58 ` Kasatkin, Dmitry
@ 2012-03-22 15:09 ` Al Viro
2012-03-22 15:19 ` Kasatkin, Dmitry
2012-03-23 14:55 ` Mimi Zohar
1 sibling, 2 replies; 26+ messages in thread
From: Al Viro @ 2012-03-22 15:09 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, linux-kernel, linux-fsdevel,
David Safford, Dmitry Kasatkin, Matt Helsley, Mimi Zohar
On Thu, Mar 22, 2012 at 10:53:04AM -0400, Mimi Zohar wrote:
> > BTW, you've missed several other places in mm/* doing fput(), so it wouldn't
> > be enough to paper over your problem anyway.
> >
> > Final fput() *can* happen under mmap_sem. Period.
>
> I think I got that loud and clear; otherwise we wouldn't have come up
> with deferring the __fput(). We have a very real problem here - writing
> extended attributes requires taking the i_mutex.
Don't do it, then? If you _must_ write to xattr on final fput, I'd suggest
starting to figure out if xattr needs its protection to be ->i_mutex - it
might make sense to introduce a separate mutex for xattr crap. Or not - I'm
not familiar enough with the guts of xattr handling in individual filesystems
to tell if that would work (e.g. if it would need unpleasant changes to
->setattr() instances)...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-22 15:09 ` Al Viro
@ 2012-03-22 15:19 ` Kasatkin, Dmitry
2012-03-22 15:39 ` Al Viro
2012-03-23 14:55 ` Mimi Zohar
1 sibling, 1 reply; 26+ messages in thread
From: Kasatkin, Dmitry @ 2012-03-22 15:19 UTC (permalink / raw)
To: Al Viro
Cc: Mimi Zohar, linux-security-module, linux-kernel, linux-fsdevel,
David Safford, Matt Helsley, Mimi Zohar
On Thu, Mar 22, 2012 at 5:09 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Mar 22, 2012 at 10:53:04AM -0400, Mimi Zohar wrote:
>> > BTW, you've missed several other places in mm/* doing fput(), so it wouldn't
>> > be enough to paper over your problem anyway.
>> >
>> > Final fput() *can* happen under mmap_sem. Period.
>>
>> I think I got that loud and clear; otherwise we wouldn't have come up
>> with deferring the __fput(). We have a very real problem here - writing
>> extended attributes requires taking the i_mutex.
>
> Don't do it, then? If you _must_ write to xattr on final fput, I'd suggest
> starting to figure out if xattr needs its protection to be ->i_mutex - it
> might make sense to introduce a separate mutex for xattr crap. Or not - I'm
"Or not" ... How to understand you?
> not familiar enough with the guts of xattr handling in individual filesystems
> to tell if that would work (e.g. if it would need unpleasant changes to
> ->setattr() instances)...
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-22 15:19 ` Kasatkin, Dmitry
@ 2012-03-22 15:39 ` Al Viro
0 siblings, 0 replies; 26+ messages in thread
From: Al Viro @ 2012-03-22 15:39 UTC (permalink / raw)
To: Kasatkin, Dmitry
Cc: Mimi Zohar, linux-security-module, linux-kernel, linux-fsdevel,
David Safford, Matt Helsley, Mimi Zohar
On Thu, Mar 22, 2012 at 05:19:09PM +0200, Kasatkin, Dmitry wrote:
> On Thu, Mar 22, 2012 at 5:09 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Thu, Mar 22, 2012 at 10:53:04AM -0400, Mimi Zohar wrote:
> >> > BTW, you've missed several other places in mm/* doing fput(), so it wouldn't
> >> > be enough to paper over your problem anyway.
> >> >
> >> > Final fput() *can* happen under mmap_sem. ??Period.
> >>
> >> I think I got that loud and clear; otherwise we wouldn't have come up
> >> with deferring the __fput(). ??We have a very real problem here - writing
> >> extended attributes requires taking the i_mutex.
> >
> > Don't do it, then? ??If you _must_ write to xattr on final fput, I'd suggest
> > starting to figure out if xattr needs its protection to be ->i_mutex - it
> > might make sense to introduce a separate mutex for xattr crap. ??Or not - I'm
>
> "Or not" ... How to understand you?
"Or it might not make sense to go that way"
> > not familiar enough with the guts of xattr handling in individual filesystems
> > to tell if that would work (e.g. if it would need unpleasant changes to
> > ->setattr() instances)...
IOW, you'll need to do quite a bit of code review to tell if it's a feasible
direction or not - I can't tell without doing the same amount of RTFS; look
for the places where xattrs are modified by fs code, see how far is ->i_mutex
acquired, whether xattrs are read in the same section and whether we rely on
->i_mutex to keep the xattr values unchanged between two reads or write and
read...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 10/12] ima: defer calling __fput()
2012-03-22 15:09 ` Al Viro
2012-03-22 15:19 ` Kasatkin, Dmitry
@ 2012-03-23 14:55 ` Mimi Zohar
1 sibling, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-23 14:55 UTC (permalink / raw)
To: Al Viro
Cc: linux-security-module, linux-kernel, linux-fsdevel,
David Safford, Dmitry Kasatkin, Matt Helsley, Mimi Zohar
On Thu, 2012-03-22 at 15:09 +0000, Al Viro wrote:
> On Thu, Mar 22, 2012 at 10:53:04AM -0400, Mimi Zohar wrote:
> > > BTW, you've missed several other places in mm/* doing fput(), so it wouldn't
> > > be enough to paper over your problem anyway.
> > >
> > > Final fput() *can* happen under mmap_sem. Period.
> >
> > I think I got that loud and clear; otherwise we wouldn't have come up
> > with deferring the __fput(). We have a very real problem here - writing
> > extended attributes requires taking the i_mutex.
>
> Don't do it, then? If you _must_ write to xattr on final fput, I'd suggest
> starting to figure out if xattr needs its protection to be ->i_mutex - it
> might make sense to introduce a separate mutex for xattr crap. Or not - I'm
> not familiar enough with the guts of xattr handling in individual filesystems
> to tell if that would work (e.g. if it would need unpleasant changes to
> ->setattr() instances)...
After looking into this, the individual filesystems do their own xattr
locking. The i_mutex, however, is currently required to access
inode->i_op->setxattr() (and the isec). In addition, IMA-appraisal
requires the i_mutex in order to calculate the file hash.
Calling ima_file_free() after the mmap_sem is released, as opposed to
queueing the __fput(), won't work, as the file needs to be open in order
to calculate the file hash.
Calling ima_file_free() before taking the mmap_sem, could work, but at
that point we don't have access to the file handle. Do you see any other
options?
thanks,
Mimi
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 11/12] ima: add support for different security.ima data types
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (9 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 10/12] ima: defer calling __fput() Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
2012-03-21 18:54 ` [PATCH v3 12/12] ima: digital signature verification support Mimi Zohar
11 siblings, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, linux-kernel, linux-fsdevel, Al Viro, David Safford,
Dmitry Kasatkin, Dmitry Kasatkin
IMA-appraisal currently verifies the integrity of a file based on a
known 'good' measurement value. This patch reserves the first byte
of 'security.ima' as a place holder for the type of method used for
verifying file data integrity.
Changelog v1:
- Use the newly defined 'struct evm_ima_xattr_data'
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@nokia.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
security/integrity/ima/ima_api.c | 6 +++---
security/integrity/ima/ima_appraise.c | 23 +++++++++++++----------
security/integrity/integrity.h | 2 +-
3 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 55deeb1..b5cbef5 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -147,8 +147,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
if (!(iint->flags & IMA_COLLECTED)) {
u64 i_version = file->f_dentry->d_inode->i_version;
- memset(iint->digest, 0, IMA_DIGEST_SIZE);
- result = ima_calc_hash(file, iint->digest);
+ iint->ima_xattr.type = IMA_XATTR_DIGEST;
+ result = ima_calc_hash(file, iint->ima_xattr.digest);
if (!result) {
iint->version = i_version;
iint->flags |= IMA_COLLECTED;
@@ -196,7 +196,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
return;
}
memset(&entry->template, 0, sizeof(entry->template));
- memcpy(entry->template.digest, iint->digest, IMA_DIGEST_SIZE);
+ memcpy(entry->template.digest, iint->ima_xattr.digest, IMA_DIGEST_SIZE);
strncpy(entry->template.file_name, filename, IMA_EVENT_NAME_LEN_MAX);
result = ima_store_template(entry, violation, inode);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a100a3c..f275d3f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -46,9 +46,9 @@ int ima_must_appraise(struct inode *inode, enum ima_hooks func, int mask)
static void ima_fix_xattr(struct dentry *dentry,
struct integrity_iint_cache *iint)
{
- iint->digest[0] = IMA_XATTR_DIGEST;
- __vfs_setxattr_noperm(dentry, XATTR_NAME_IMA,
- iint->digest, IMA_DIGEST_SIZE + 1, 0);
+ iint->ima_xattr.type = IMA_XATTR_DIGEST;
+ __vfs_setxattr_noperm(dentry, XATTR_NAME_IMA, (u8 *)&iint->ima_xattr,
+ sizeof iint->ima_xattr, 0);
}
/*
@@ -64,7 +64,7 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
{
struct dentry *dentry = file->f_dentry;
struct inode *inode = dentry->d_inode;
- u8 xattr_value[IMA_DIGEST_SIZE];
+ struct evm_ima_xattr_data xattr_value;
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = "appraise_data";
char *cause = "unknown";
@@ -78,8 +78,8 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
if (iint->flags & IMA_APPRAISED)
return iint->ima_status;
- rc = inode->i_op->getxattr(dentry, XATTR_NAME_IMA, xattr_value,
- IMA_DIGEST_SIZE);
+ rc = inode->i_op->getxattr(dentry, XATTR_NAME_IMA, (u8 *)&xattr_value,
+ sizeof xattr_value);
if (rc <= 0) {
if (rc && rc != -ENODATA)
goto out;
@@ -90,7 +90,8 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
goto out;
}
- status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
+ status = evm_verifyxattr(dentry, XATTR_NAME_IMA, (u8 *)&xattr_value,
+ rc, iint);
if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
if ((status == INTEGRITY_NOLABEL)
|| (status == INTEGRITY_NOXATTRS))
@@ -100,14 +101,16 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
goto out;
}
- rc = memcmp(xattr_value, iint->digest, IMA_DIGEST_SIZE);
+ rc = memcmp(xattr_value.digest, iint->ima_xattr.digest,
+ IMA_DIGEST_SIZE);
if (rc) {
status = INTEGRITY_FAIL;
cause = "invalid-hash";
print_hex_dump_bytes("security.ima: ", DUMP_PREFIX_NONE,
- xattr_value, IMA_DIGEST_SIZE);
+ &xattr_value, sizeof xattr_value);
print_hex_dump_bytes("collected: ", DUMP_PREFIX_NONE,
- iint->digest, IMA_DIGEST_SIZE);
+ (u8 *)&iint->ima_xattr,
+ sizeof iint->ima_xattr);
goto out;
}
status = INTEGRITY_PASS;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 295702d..c145331 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -39,7 +39,7 @@ struct integrity_iint_cache {
struct inode *inode; /* back pointer to inode in question */
u64 version; /* track inode changes */
unsigned char flags;
- u8 digest[SHA1_DIGEST_SIZE];
+ struct evm_ima_xattr_data ima_xattr;
enum integrity_status ima_status;
enum integrity_status evm_status;
};
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 12/12] ima: digital signature verification support
2012-03-21 18:54 [PATCH v3 00/12] ima: appraisal extension Mimi Zohar
` (10 preceding siblings ...)
2012-03-21 18:54 ` [PATCH v3 11/12] ima: add support for different security.ima data types Mimi Zohar
@ 2012-03-21 18:54 ` Mimi Zohar
11 siblings, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2012-03-21 18:54 UTC (permalink / raw)
To: linux-security-module
Cc: Dmitry Kasatkin, linux-kernel, linux-fsdevel, Al Viro,
David Safford, Mimi Zohar
From: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
This patch adds support for digital signature based integrity appraisal.
With this patch, 'security.ima' contains either the file data hash or
a digital signature of the file data hash. The file data hash provides
the security attribute of file integrity. In addition to file integrity,
a digital signature provides the security attribute of authenticity.
Unlike EVM, when the file metadata changes, the digital signature is
replaced with an HMAC, modification of the file data does not cause the
'security.ima' digital signature to be replaced with a hash. As a
result, after any modification, subsequent file integrity appraisals
would fail.
Although digitally signed files can be modified, but by not updating
'security.ima' to reflect these modifications, in essence digitally
signed files could be considered 'immutable'.
IMA uses a different keyring than EVM. While the EVM keyring should not
be updated after initialization and locked, the IMA keyring should allow
updating or adding new keys when upgrading or installing packages.
Changelog v3:
- Permit files without any 'security.ima' xattr to be labeled properly.
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
security/integrity/ima/ima_appraise.c | 70 ++++++++++++++++++++++++---------
security/integrity/integrity.h | 1 +
2 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index f275d3f..522524c 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -64,7 +64,7 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
{
struct dentry *dentry = file->f_dentry;
struct inode *inode = dentry->d_inode;
- struct evm_ima_xattr_data xattr_value;
+ struct evm_ima_xattr_data *xattr_value = NULL;
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = "appraise_data";
char *cause = "unknown";
@@ -78,8 +78,8 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
if (iint->flags & IMA_APPRAISED)
return iint->ima_status;
- rc = inode->i_op->getxattr(dentry, XATTR_NAME_IMA, (u8 *)&xattr_value,
- sizeof xattr_value);
+ rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)&xattr_value,
+ 0, GFP_NOFS);
if (rc <= 0) {
if (rc && rc != -ENODATA)
goto out;
@@ -90,8 +90,7 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
goto out;
}
- status = evm_verifyxattr(dentry, XATTR_NAME_IMA, (u8 *)&xattr_value,
- rc, iint);
+ status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
if ((status == INTEGRITY_NOLABEL)
|| (status == INTEGRITY_NOXATTRS))
@@ -101,30 +100,58 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
goto out;
}
- rc = memcmp(xattr_value.digest, iint->ima_xattr.digest,
- IMA_DIGEST_SIZE);
- if (rc) {
- status = INTEGRITY_FAIL;
- cause = "invalid-hash";
- print_hex_dump_bytes("security.ima: ", DUMP_PREFIX_NONE,
- &xattr_value, sizeof xattr_value);
- print_hex_dump_bytes("collected: ", DUMP_PREFIX_NONE,
- (u8 *)&iint->ima_xattr,
- sizeof iint->ima_xattr);
- goto out;
+ switch (xattr_value->type) {
+ case IMA_XATTR_DIGEST:
+ rc = memcmp(xattr_value->digest, iint->ima_xattr.digest,
+ IMA_DIGEST_SIZE);
+ if (rc) {
+ cause = "invalid-hash";
+ status = INTEGRITY_FAIL;
+ print_hex_dump_bytes("security.ima: ", DUMP_PREFIX_NONE,
+ xattr_value, sizeof(*xattr_value));
+ print_hex_dump_bytes("collected: ", DUMP_PREFIX_NONE,
+ (u8 *)&iint->ima_xattr,
+ sizeof iint->ima_xattr);
+ break;
+ }
+ status = INTEGRITY_PASS;
+ break;
+ case EVM_IMA_XATTR_DIGSIG:
+ iint->flags |= IMA_DIGSIG;
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
+ xattr_value->digest, rc - 1,
+ iint->ima_xattr.digest,
+ IMA_DIGEST_SIZE);
+ if (rc == -EOPNOTSUPP) {
+ status = INTEGRITY_UNKNOWN;
+ } else if (rc) {
+ cause = "invalid-signature";
+ status = INTEGRITY_FAIL;
+ } else {
+ status = INTEGRITY_PASS;
+ }
+ break;
+ default:
+ status = INTEGRITY_UNKNOWN;
+ cause = "unknown-ima-data";
+ break;
}
- status = INTEGRITY_PASS;
- iint->flags |= IMA_APPRAISED;
+
out:
if (status != INTEGRITY_PASS) {
- if (ima_appraise & IMA_APPRAISE_FIX) {
+ if ((ima_appraise & IMA_APPRAISE_FIX) &&
+ (!xattr_value ||
+ xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
ima_fix_xattr(dentry, iint);
status = INTEGRITY_PASS;
}
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
op, cause, rc, 0);
+ } else {
+ iint->flags |= IMA_APPRAISED;
}
iint->ima_status = status;
+ kfree(xattr_value);
return status;
}
@@ -136,9 +163,14 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
struct dentry *dentry = file->f_dentry;
int rc = 0;
+ /* do not collect and update hash for digital signatures */
+ if (iint->flags & IMA_DIGSIG)
+ return;
+
rc = ima_collect_measurement(iint, file);
if (rc < 0)
return;
+
ima_fix_xattr(dentry, iint);
}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index c145331..0594a57 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -21,6 +21,7 @@
#define IMA_APPRAISE 4
#define IMA_APPRAISED 8
#define IMA_COLLECTED 16
+#define IMA_DIGSIG 32
enum evm_ima_xattr_type {
IMA_XATTR_DIGEST = 0x01,
--
1.7.6.5
^ permalink raw reply related [flat|nested] 26+ messages in thread