[PATCH 0/8] integrity: miscellaneous cleanups

[PATCH 0/8] integrity: miscellaneous cleanups

[Dmitry Kasatkin]

Hi,

Here is a few miscellaneous cleanups to improve code quality,
performance and prevent unnecessary memory allocations.

- Dmitry

Dmitry Kasatkin (8):
  integrity: prevent flooding with 'Request for unknown key'
  integrity: remove declaration of non-existing functions
  ima: simplify conditional statement to improve performance
  ima: remove unnecessary extra variable
  ima: add missing '__init' keywords
  ima: remove unnecessary code
  ima: remove usage of filename parameter
  ima: initialize only required template

 security/integrity/digsig_asymmetric.c |  5 +++--
 security/integrity/ima/ima.h           | 11 -----------
 security/integrity/ima/ima_appraise.c  |  2 --
 security/integrity/ima/ima_crypto.c    |  2 +-
 security/integrity/ima/ima_main.c      | 34 +++++++++++++++-------------------
 security/integrity/ima/ima_template.c  | 30 +++++-------------------------
 security/integrity/integrity.h         |  1 -
 7 files changed, 24 insertions(+), 61 deletions(-)

-- 
1.9.1

[PATCH 1/8] integrity: prevent flooding with 'Request for unknown key'

[Dmitry Kasatkin]

If file has IMA signature, IMA in enforce mode, but key is missing
then file access is blocked and single error message is printed.

If IMA appraisal is enabled in fix mode, then system runs as usual
but might produce tons of 'Request for unknown key' messages.

This patch switches 'pr_warn' to 'pr_err_ratelimited'.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/digsig_asymmetric.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index 9eae480..37e0d98 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -13,6 +13,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/err.h>
+#include <linux/ratelimit.h>
 #include <linux/key-type.h>
 #include <crypto/public_key.h>
 #include <keys/asymmetric-type.h>
@@ -45,8 +46,8 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
 	}
 
 	if (IS_ERR(key)) {
-		pr_warn("Request for unknown key '%s' err %ld\n",
-			name, PTR_ERR(key));
+		pr_err_ratelimited("Request for unknown key '%s' err %ld\n",
+				   name, PTR_ERR(key));
 		switch (PTR_ERR(key)) {
 			/* Hide some search errors */
 		case -EACCES:
-- 
1.9.1

[PATCH 2/8] integrity: remove declaration of non-existing functions

[Dmitry Kasatkin]

Noticed that there are declaration of few non-existing functions.
Also remove duplicated declaration of inegrity_iint_find().

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima.h   | 9 ---------
 security/integrity/integrity.h | 1 -
 2 files changed, 10 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 0fb456c..c6990a7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -90,10 +90,7 @@ extern struct list_head ima_measurements;	/* list of all measurements */
 
 /* Internal IMA function definitions */
 int ima_init(void);
-void ima_cleanup(void);
 int ima_fs_init(void);
-void ima_fs_cleanup(void);
-int ima_inode_alloc(struct inode *inode);
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename);
@@ -151,12 +148,6 @@ int ima_store_template(struct ima_template_entry *entry, int violation,
 void ima_free_template_entry(struct ima_template_entry *entry);
 const char *ima_d_path(struct path *path, char **pathbuf);
 
-/* rbtree tree calls to lookup, insert, delete
- * integrity data associated with an inode.
- */
-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, MMAP_CHECK, BPRM_CHECK, MODULE_CHECK, FIRMWARE_CHECK, POST_SETATTR };
 
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 904e68a..c0379d1 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -117,7 +117,6 @@ struct integrity_iint_cache {
 /* rbtree tree calls to lookup, insert, delete
  * integrity data associated with an inode.
  */
-struct integrity_iint_cache *integrity_iint_insert(struct inode *inode);
 struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 
 #define INTEGRITY_KEYRING_EVM		0
-- 
1.9.1

[PATCH 3/8] ima: simplify conditional statement to improve performance

[Dmitry Kasatkin]

Precede bit testing before string comparison makes code
faster. Also refactor statement as a single line pointer
assignment.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f82cf9b..f7b85bf 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -206,10 +206,8 @@ static int process_measurement(struct file *file, const char *filename,
 	}
 
 	template_desc = ima_template_desc_current();
-	if (strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0) {
-		if (action & IMA_APPRAISE_SUBMASK)
-			xattr_ptr = &xattr_value;
-	} else
+	if ((action & IMA_APPRAISE_SUBMASK) ||
+		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
 		xattr_ptr = &xattr_value;
 
 	rc = ima_collect_measurement(iint, file, xattr_ptr, &xattr_len);
-- 
1.9.1

[PATCH 4/8] ima: remove unnecessary extra variable

[Dmitry Kasatkin]

'function' variable value can be changed instead of
allocating extra '_func' variable.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f7b85bf..aaf5552 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -164,7 +164,7 @@ static int process_measurement(struct file *file, const char *filename,
 	struct ima_template_desc *template_desc;
 	char *pathbuf = NULL;
 	const char *pathname = NULL;
-	int rc = -ENOMEM, action, must_appraise, _func;
+	int rc = -ENOMEM, action, must_appraise;
 	struct evm_ima_xattr_data *xattr_value = NULL, **xattr_ptr = NULL;
 	int xattr_len = 0;
 
@@ -182,7 +182,8 @@ static int process_measurement(struct file *file, const char *filename,
 	must_appraise = action & IMA_APPRAISE;
 
 	/*  Is the appraise rule hook specific?  */
-	_func = (action & IMA_FILE_APPRAISE) ? FILE_CHECK : function;
+	if (action & IMA_FILE_APPRAISE)
+		function = FILE_CHECK;
 
 	mutex_lock(&inode->i_mutex);
 
@@ -201,7 +202,7 @@ static int process_measurement(struct file *file, const char *filename,
 	/* Nothing to do, just return existing appraised status */
 	if (!action) {
 		if (must_appraise)
-			rc = ima_get_cache_status(iint, _func);
+			rc = ima_get_cache_status(iint, function);
 		goto out_digsig;
 	}
 
@@ -223,7 +224,7 @@ static int process_measurement(struct file *file, const char *filename,
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len);
 	if (action & IMA_APPRAISE_SUBMASK)
-		rc = ima_appraise_measurement(_func, iint, file, pathname,
+		rc = ima_appraise_measurement(function, iint, file, pathname,
 					      xattr_value, xattr_len, opened);
 	if (action & IMA_AUDIT)
 		ima_audit_measurement(iint, pathname);
-- 
1.9.1

[PATCH 5/8] ima: add missing '__init' keywords

[Dmitry Kasatkin]

Add missing keywords to the function definition to cleanup
to discard initialization code.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima.h          | 2 --
 security/integrity/ima/ima_crypto.c   | 2 +-
 security/integrity/ima/ima_template.c | 4 ++--
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c6990a7..8e4bb88 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -107,8 +107,6 @@ void ima_print_digest(struct seq_file *m, u8 *digest, int size);
 struct ima_template_desc *ima_template_desc_current(void);
 int ima_init_template(void);
 
-int ima_init_template(void);
-
 /*
  * used to protect h_table and sha_table
  */
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 3b26472..d34e7df 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -97,7 +97,7 @@ static int ima_kernel_read(struct file *file, loff_t offset,
 	return ret;
 }
 
-int ima_init_crypto(void)
+int __init ima_init_crypto(void)
 {
 	long rc;
 
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index a076a96..f682606 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -152,7 +152,7 @@ out:
 	return result;
 }
 
-static int init_defined_templates(void)
+static int __init init_defined_templates(void)
 {
 	int i = 0;
 	int result = 0;
@@ -178,7 +178,7 @@ struct ima_template_desc *ima_template_desc_current(void)
 	return ima_template;
 }
 
-int ima_init_template(void)
+int __init ima_init_template(void)
 {
 	int result;
 
-- 
1.9.1

[PATCH 6/8] ima: remove unnecessary code

[Dmitry Kasatkin]

If ima_appraise is 0, then action would not mandate to perform
appraisal and ima_appraise_measurement will never be called.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_appraise.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 225fd94..013ec3f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -192,8 +192,6 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 	enum integrity_status status = INTEGRITY_UNKNOWN;
 	int rc = xattr_len, hash_start = 0;
 
-	if (!ima_appraise)
-		return 0;
 	if (!inode->i_op->getxattr)
 		return INTEGRITY_UNKNOWN;
 
-- 
1.9.1

[PATCH 7/8] ima: remove usage of filename parameter

[Dmitry Kasatkin]

In all cases except ima_bprm_check() filename was not defined and
ima_d_path() was used to find full path.

ima_bprm_check() used to select between bprm->interp and bprm->filename.
Following dump demonstrates differences between using filename and interp.

bprm->filename
 filename: ./foo.sh, pathname: /root/bin/foo.sh
 filename: ./foo.sh, pathname: /bin/dash

bprm->interp
 filename: ./foo.sh, pathname: /root/bin/foo.sh
 filename: /bin/sh, pathname: /bin/dash

In both cases pathnames are the same.
This patch removes usage of filename and interp in favor of d_path.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_main.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index aaf5552..673a37e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -156,8 +156,8 @@ void ima_file_free(struct file *file)
 	ima_check_last_writer(iint, inode, file);
 }
 
-static int process_measurement(struct file *file, const char *filename,
-			       int mask, int function, int opened)
+static int process_measurement(struct file *file, int mask, int function,
+			       int opened)
 {
 	struct inode *inode = file_inode(file);
 	struct integrity_iint_cache *iint;
@@ -218,7 +218,7 @@ static int process_measurement(struct file *file, const char *filename,
 		goto out_digsig;
 	}
 
-	pathname = filename ?: ima_d_path(&file->f_path, &pathbuf);
+	pathname = ima_d_path(&file->f_path, &pathbuf);
 
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
@@ -254,7 +254,7 @@ out:
 int ima_file_mmap(struct file *file, unsigned long prot)
 {
 	if (file && (prot & PROT_EXEC))
-		return process_measurement(file, NULL, MAY_EXEC, MMAP_CHECK, 0);
+		return process_measurement(file, MAY_EXEC, MMAP_CHECK, 0);
 	return 0;
 }
 
@@ -273,10 +273,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
  */
 int ima_bprm_check(struct linux_binprm *bprm)
 {
-	return process_measurement(bprm->file,
-				   (strcmp(bprm->filename, bprm->interp) == 0) ?
-				   bprm->filename : bprm->interp,
-				   MAY_EXEC, BPRM_CHECK, 0);
+	return process_measurement(bprm->file, MAY_EXEC, BPRM_CHECK, 0);
 }
 
 /**
@@ -292,7 +289,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
 int ima_file_check(struct file *file, int mask, int opened)
 {
 	ima_rdwr_violation_check(file);
-	return process_measurement(file, NULL,
+	return process_measurement(file,
 				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
 				   FILE_CHECK, opened);
 }
@@ -317,7 +314,7 @@ int ima_module_check(struct file *file)
 #endif
 		return 0;	/* We rely on module signature checking */
 	}
-	return process_measurement(file, NULL, MAY_EXEC, MODULE_CHECK, 0);
+	return process_measurement(file, MAY_EXEC, MODULE_CHECK, 0);
 }
 
 int ima_fw_from_file(struct file *file, char *buf, size_t size)
@@ -328,7 +325,7 @@ int ima_fw_from_file(struct file *file, char *buf, size_t size)
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
 		return 0;
 	}
-	return process_measurement(file, NULL, MAY_EXEC, FIRMWARE_CHECK, 0);
+	return process_measurement(file, MAY_EXEC, FIRMWARE_CHECK, 0);
 }
 
 static int __init init_ima(void)
-- 
1.9.1

[PATCH 8/8] ima: initialize only required template

[Dmitry Kasatkin]

IMA uses only one template. This patch initializes only required
template to avoid unnecessary memory allocations.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_template.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index f682606..e854862 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -152,24 +152,6 @@ out:
 	return result;
 }
 
-static int __init init_defined_templates(void)
-{
-	int i = 0;
-	int result = 0;
-
-	/* Init defined templates. */
-	for (i = 0; i < ARRAY_SIZE(defined_templates); i++) {
-		struct ima_template_desc *template = &defined_templates[i];
-
-		result = template_desc_init_fields(template->fmt,
-						   &(template->fields),
-						   &(template->num_fields));
-		if (result < 0)
-			return result;
-	}
-	return result;
-}
-
 struct ima_template_desc *ima_template_desc_current(void)
 {
 	if (!ima_template)
@@ -180,11 +162,9 @@ struct ima_template_desc *ima_template_desc_current(void)
 
 int __init ima_init_template(void)
 {
-	int result;
-
-	result = init_defined_templates();
-	if (result < 0)
-		return result;
+	struct ima_template_desc *template = ima_template_desc_current();
 
-	return 0;
+	return template_desc_init_fields(template->fmt,
+					 &(template->fields),
+					 &(template->num_fields));
 }
-- 
1.9.1

Re: [PATCH 2/8] integrity: remove declaration of non-existing functions

[Mimi Zohar]

On Wed, 2014-09-03 at 10:19 +0300, Dmitry Kasatkin wrote: 
> Noticed that there are declaration of few non-existing functions.
> Also remove duplicated declaration of inegrity_iint_find().

Please include the commits, which removed these functions, in the patch
description.

Mimi
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>  security/integrity/ima/ima.h   | 9 ---------
>  security/integrity/integrity.h | 1 -
>  2 files changed, 10 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 0fb456c..c6990a7 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -90,10 +90,7 @@ extern struct list_head ima_measurements;	/* list of all measurements */
> 
>  /* Internal IMA function definitions */
>  int ima_init(void);
> -void ima_cleanup(void);
>  int ima_fs_init(void);
> -void ima_fs_cleanup(void);
> -int ima_inode_alloc(struct inode *inode);
>  int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>  			   const char *op, struct inode *inode,
>  			   const unsigned char *filename);
> @@ -151,12 +148,6 @@ int ima_store_template(struct ima_template_entry *entry, int violation,
>  void ima_free_template_entry(struct ima_template_entry *entry);
>  const char *ima_d_path(struct path *path, char **pathbuf);
> 
> -/* rbtree tree calls to lookup, insert, delete
> - * integrity data associated with an inode.
> - */
> -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, MMAP_CHECK, BPRM_CHECK, MODULE_CHECK, FIRMWARE_CHECK, POST_SETATTR };
> 
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 904e68a..c0379d1 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -117,7 +117,6 @@ struct integrity_iint_cache {
>  /* rbtree tree calls to lookup, insert, delete
>   * integrity data associated with an inode.
>   */
> -struct integrity_iint_cache *integrity_iint_insert(struct inode *inode);
>  struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
> 
>  #define INTEGRITY_KEYRING_EVM		0

Re: [PATCH 3/8] ima: simplify conditional statement to improve performance

[Mimi Zohar]

On Wed, 2014-09-03 at 10:19 +0300, Dmitry Kasatkin wrote: 
> Precede bit testing before string comparison makes code
> faster. Also refactor statement as a single line pointer
> assignment.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>

Functionally the code hasn't change, but the resulting conditional
statement isn't as clear, at least to me, as the original code.  Please
include here in the patch description a short explanation of what the
code is doing.

thanks,

Mimi

> ---
>  security/integrity/ima/ima_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index f82cf9b..f7b85bf 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -206,10 +206,8 @@ static int process_measurement(struct file *file, const char *filename,
>  	}
> 
>  	template_desc = ima_template_desc_current();
> -	if (strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0) {
> -		if (action & IMA_APPRAISE_SUBMASK)
> -			xattr_ptr = &xattr_value;
> -	} else
> +	if ((action & IMA_APPRAISE_SUBMASK) ||
> +		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
>  		xattr_ptr = &xattr_value;
> 
>  	rc = ima_collect_measurement(iint, file, xattr_ptr, &xattr_len);

Re: [PATCH 6/8] ima: remove unnecessary code

[Mimi Zohar]

On Wed, 2014-09-03 at 10:19 +0300, Dmitry Kasatkin wrote: 
> If ima_appraise is 0, then action would not mandate to perform
> appraisal and ima_appraise_measurement will never be called.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>

The policy determines whether or not a file should be appraised.
Whether IMA is configured and enabled to appraise files is a different
issue.  The test is not done in process_measurement(), but deferred to
here.

Mimi

> ---
>  security/integrity/ima/ima_appraise.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 225fd94..013ec3f 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -192,8 +192,6 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>  	enum integrity_status status = INTEGRITY_UNKNOWN;
>  	int rc = xattr_len, hash_start = 0;
> 
> -	if (!ima_appraise)
> -		return 0;
>  	if (!inode->i_op->getxattr)
>  		return INTEGRITY_UNKNOWN;
> 

Re: [PATCH 2/8] integrity: remove declaration of non-existing functions

[Dmitry Kasatkin]

On 03/09/14 15:51, Mimi Zohar wrote:
> On Wed, 2014-09-03 at 10:19 +0300, Dmitry Kasatkin wrote: 
>> Noticed that there are declaration of few non-existing functions.
>> Also remove duplicated declaration of inegrity_iint_find().
> Please include the commits, which removed these functions, in the patch
> description.

Do you think it is really necessary?

Anyway.
f381c272224f5f158f5cff64f8f3481fa0eee8b3 integrity: move ima inode
integrity data management

- Dmitry

> Mimi
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> ---
>>  security/integrity/ima/ima.h   | 9 ---------
>>  security/integrity/integrity.h | 1 -
>>  2 files changed, 10 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 0fb456c..c6990a7 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -90,10 +90,7 @@ extern struct list_head ima_measurements;	/* list of all measurements */
>>
>>  /* Internal IMA function definitions */
>>  int ima_init(void);
>> -void ima_cleanup(void);
>>  int ima_fs_init(void);
>> -void ima_fs_cleanup(void);
>> -int ima_inode_alloc(struct inode *inode);
>>  int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>  			   const char *op, struct inode *inode,
>>  			   const unsigned char *filename);
>> @@ -151,12 +148,6 @@ int ima_store_template(struct ima_template_entry *entry, int violation,
>>  void ima_free_template_entry(struct ima_template_entry *entry);
>>  const char *ima_d_path(struct path *path, char **pathbuf);
>>
>> -/* rbtree tree calls to lookup, insert, delete
>> - * integrity data associated with an inode.
>> - */
>> -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, MMAP_CHECK, BPRM_CHECK, MODULE_CHECK, FIRMWARE_CHECK, POST_SETATTR };
>>
>> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
>> index 904e68a..c0379d1 100644
>> --- a/security/integrity/integrity.h
>> +++ b/security/integrity/integrity.h
>> @@ -117,7 +117,6 @@ struct integrity_iint_cache {
>>  /* rbtree tree calls to lookup, insert, delete
>>   * integrity data associated with an inode.
>>   */
>> -struct integrity_iint_cache *integrity_iint_insert(struct inode *inode);
>>  struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>>
>>  #define INTEGRITY_KEYRING_EVM		0
>
>

Re: [PATCH 7/8] ima: remove usage of filename parameter

[Mimi Zohar]

On Wed, 2014-09-03 at 10:20 +0300, Dmitry Kasatkin wrote: 
> In all cases except ima_bprm_check() filename was not defined and
> ima_d_path() was used to find full path.
> 
> ima_bprm_check() used to select between bprm->interp and bprm->filename.
> Following dump demonstrates differences between using filename and interp.
> 
> bprm->filename
>  filename: ./foo.sh, pathname: /root/bin/foo.sh
>  filename: ./foo.sh, pathname: /bin/dash
> 
> bprm->interp
>  filename: ./foo.sh, pathname: /root/bin/foo.sh
>  filename: /bin/sh, pathname: /bin/dash
> 
> In both cases pathnames are the same.
> This patch removes usage of filename and interp in favor of d_path.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>

Thanks, this has been on my list to do.  My only concern is whether we
should be using d_path() or one of the other variants (eg.
dentry_path(), d_absolute_path()).  For namespaces, we would want to be
able to differentiate the files.

Please include in this patch description why d_path(), if it is the
case, the best option.

thanks,

Mimi

> ---
>  security/integrity/ima/ima_main.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index aaf5552..673a37e 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -156,8 +156,8 @@ void ima_file_free(struct file *file)
>  	ima_check_last_writer(iint, inode, file);
>  }
> 
> -static int process_measurement(struct file *file, const char *filename,
> -			       int mask, int function, int opened)
> +static int process_measurement(struct file *file, int mask, int function,
> +			       int opened)
>  {
>  	struct inode *inode = file_inode(file);
>  	struct integrity_iint_cache *iint;
> @@ -218,7 +218,7 @@ static int process_measurement(struct file *file, const char *filename,
>  		goto out_digsig;
>  	}
> 
> -	pathname = filename ?: ima_d_path(&file->f_path, &pathbuf);
> +	pathname = ima_d_path(&file->f_path, &pathbuf);
> 
>  	if (action & IMA_MEASURE)
>  		ima_store_measurement(iint, file, pathname,
> @@ -254,7 +254,7 @@ out:
>  int ima_file_mmap(struct file *file, unsigned long prot)
>  {
>  	if (file && (prot & PROT_EXEC))
> -		return process_measurement(file, NULL, MAY_EXEC, MMAP_CHECK, 0);
> +		return process_measurement(file, MAY_EXEC, MMAP_CHECK, 0);
>  	return 0;
>  }
> 
> @@ -273,10 +273,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
>   */
>  int ima_bprm_check(struct linux_binprm *bprm)
>  {
> -	return process_measurement(bprm->file,
> -				   (strcmp(bprm->filename, bprm->interp) == 0) ?
> -				   bprm->filename : bprm->interp,
> -				   MAY_EXEC, BPRM_CHECK, 0);
> +	return process_measurement(bprm->file, MAY_EXEC, BPRM_CHECK, 0);
>  }
> 
>  /**
> @@ -292,7 +289,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
>  int ima_file_check(struct file *file, int mask, int opened)
>  {
>  	ima_rdwr_violation_check(file);
> -	return process_measurement(file, NULL,
> +	return process_measurement(file,
>  				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
>  				   FILE_CHECK, opened);
>  }
> @@ -317,7 +314,7 @@ int ima_module_check(struct file *file)
>  #endif
>  		return 0;	/* We rely on module signature checking */
>  	}
> -	return process_measurement(file, NULL, MAY_EXEC, MODULE_CHECK, 0);
> +	return process_measurement(file, MAY_EXEC, MODULE_CHECK, 0);
>  }
> 
>  int ima_fw_from_file(struct file *file, char *buf, size_t size)
> @@ -328,7 +325,7 @@ int ima_fw_from_file(struct file *file, char *buf, size_t size)
>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
>  		return 0;
>  	}
> -	return process_measurement(file, NULL, MAY_EXEC, FIRMWARE_CHECK, 0);
> +	return process_measurement(file, MAY_EXEC, FIRMWARE_CHECK, 0);
>  }
> 
>  static int __init init_ima(void)

Re: [PATCH 7/8] ima: remove usage of filename parameter

[Dmitry Kasatkin]

On 03/09/14 16:16, Mimi Zohar wrote:
> On Wed, 2014-09-03 at 10:20 +0300, Dmitry Kasatkin wrote: 
>> In all cases except ima_bprm_check() filename was not defined and
>> ima_d_path() was used to find full path.
>>
>> ima_bprm_check() used to select between bprm->interp and bprm->filename.
>> Following dump demonstrates differences between using filename and interp.
>>
>> bprm->filename
>>  filename: ./foo.sh, pathname: /root/bin/foo.sh
>>  filename: ./foo.sh, pathname: /bin/dash
>>
>> bprm->interp
>>  filename: ./foo.sh, pathname: /root/bin/foo.sh
>>  filename: /bin/sh, pathname: /bin/dash
>>
>> In both cases pathnames are the same.
>> This patch removes usage of filename and interp in favor of d_path.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> Thanks, this has been on my list to do.  My only concern is whether we
> should be using d_path() or one of the other variants (eg.
> dentry_path(), d_absolute_path()).  For namespaces, we would want to be
> able to differentiate the files.
>
> Please include in this patch description why d_path(), if it is the
> case, the best option.
>
> thanks,

Hi,

Actually as we discussed, we can also in this patch change ima_d_path to
use d_absolute_path().
It will work for "chroot" cases and will show real path...

Should I switch to 'd_absolute_path'?

In the case of namespaces, neither d_path nor d_absolute_path works....
Usage of dentry_path() would eliminate mount tree and requires device
prefix.
But it will 'break' clients, reading process measurement list.
That would require essentially more agreement.

- Dmitry


> Mimi
>
>> ---
>>  security/integrity/ima/ima_main.c | 19 ++++++++-----------
>>  1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index aaf5552..673a37e 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -156,8 +156,8 @@ void ima_file_free(struct file *file)
>>  	ima_check_last_writer(iint, inode, file);
>>  }
>>
>> -static int process_measurement(struct file *file, const char *filename,
>> -			       int mask, int function, int opened)
>> +static int process_measurement(struct file *file, int mask, int function,
>> +			       int opened)
>>  {
>>  	struct inode *inode = file_inode(file);
>>  	struct integrity_iint_cache *iint;
>> @@ -218,7 +218,7 @@ static int process_measurement(struct file *file, const char *filename,
>>  		goto out_digsig;
>>  	}
>>
>> -	pathname = filename ?: ima_d_path(&file->f_path, &pathbuf);
>> +	pathname = ima_d_path(&file->f_path, &pathbuf);
>>
>>  	if (action & IMA_MEASURE)
>>  		ima_store_measurement(iint, file, pathname,
>> @@ -254,7 +254,7 @@ out:
>>  int ima_file_mmap(struct file *file, unsigned long prot)
>>  {
>>  	if (file && (prot & PROT_EXEC))
>> -		return process_measurement(file, NULL, MAY_EXEC, MMAP_CHECK, 0);
>> +		return process_measurement(file, MAY_EXEC, MMAP_CHECK, 0);
>>  	return 0;
>>  }
>>
>> @@ -273,10 +273,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
>>   */
>>  int ima_bprm_check(struct linux_binprm *bprm)
>>  {
>> -	return process_measurement(bprm->file,
>> -				   (strcmp(bprm->filename, bprm->interp) == 0) ?
>> -				   bprm->filename : bprm->interp,
>> -				   MAY_EXEC, BPRM_CHECK, 0);
>> +	return process_measurement(bprm->file, MAY_EXEC, BPRM_CHECK, 0);
>>  }
>>
>>  /**
>> @@ -292,7 +289,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
>>  int ima_file_check(struct file *file, int mask, int opened)
>>  {
>>  	ima_rdwr_violation_check(file);
>> -	return process_measurement(file, NULL,
>> +	return process_measurement(file,
>>  				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
>>  				   FILE_CHECK, opened);
>>  }
>> @@ -317,7 +314,7 @@ int ima_module_check(struct file *file)
>>  #endif
>>  		return 0;	/* We rely on module signature checking */
>>  	}
>> -	return process_measurement(file, NULL, MAY_EXEC, MODULE_CHECK, 0);
>> +	return process_measurement(file, MAY_EXEC, MODULE_CHECK, 0);
>>  }
>>
>>  int ima_fw_from_file(struct file *file, char *buf, size_t size)
>> @@ -328,7 +325,7 @@ int ima_fw_from_file(struct file *file, char *buf, size_t size)
>>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
>>  		return 0;
>>  	}
>> -	return process_measurement(file, NULL, MAY_EXEC, FIRMWARE_CHECK, 0);
>> +	return process_measurement(file, MAY_EXEC, FIRMWARE_CHECK, 0);
>>  }
>>
>>  static int __init init_ima(void)
>
>

Re: [PATCH 6/8] ima: remove unnecessary code

[Dmitry Kasatkin]

On 03/09/14 16:08, Mimi Zohar wrote:
> On Wed, 2014-09-03 at 10:19 +0300, Dmitry Kasatkin wrote: 
>> If ima_appraise is 0, then action would not mandate to perform
>> appraisal and ima_appraise_measurement will never be called.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> The policy determines whether or not a file should be appraised.
> Whether IMA is configured and enabled to appraise files is a different
> issue.  The test is not done in process_measurement(), but deferred to
> here.

Hi,

Policy requests honors "ima_appraise" variable.
There wont be any appraisal action if 'ima_appraise' is disabled.

See bellow...

--------------------
int ima_get_action(struct inode *inode, int mask, int function)
{
        .........
        if (!ima_appraise)
                flags &= ~IMA_APPRAISE;
        ..........
--------------

- Dmitry
> Mimi
>
>> ---
>>  security/integrity/ima/ima_appraise.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> index 225fd94..013ec3f 100644
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -192,8 +192,6 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>>  	enum integrity_status status = INTEGRITY_UNKNOWN;
>>  	int rc = xattr_len, hash_start = 0;
>>
>> -	if (!ima_appraise)
>> -		return 0;
>>  	if (!inode->i_op->getxattr)
>>  		return INTEGRITY_UNKNOWN;
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [Linux-ima-devel] [PATCH 8/8] ima: initialize only required template

[Roberto Sassu]

On 09/03/2014 09:20 AM, Dmitry Kasatkin wrote:
> IMA uses only one template. This patch initializes only required
> template to avoid unnecessary memory allocations.
>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>   security/integrity/ima/ima_template.c | 28 ++++------------------------
>   1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index f682606..e854862 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -152,24 +152,6 @@ out:
>   	return result;
>   }
>
> -static int __init init_defined_templates(void)
> -{
> -	int i = 0;
> -	int result = 0;
> -
> -	/* Init defined templates. */
> -	for (i = 0; i < ARRAY_SIZE(defined_templates); i++) {
> -		struct ima_template_desc *template = &defined_templates[i];
> -
> -		result = template_desc_init_fields(template->fmt,
> -						   &(template->fields),
> -						   &(template->num_fields));
> -		if (result < 0)
> -			return result;
> -	}
> -	return result;
> -}
> -
>   struct ima_template_desc *ima_template_desc_current(void)
>   {
>   	if (!ima_template)
> @@ -180,11 +162,9 @@ struct ima_template_desc *ima_template_desc_current(void)
>
>   int __init ima_init_template(void)
>   {
> -	int result;
> -
> -	result = init_defined_templates();
> -	if (result < 0)
> -		return result;
> +	struct ima_template_desc *template = ima_template_desc_current();
>
> -	return 0;
> +	return template_desc_init_fields(template->fmt,
> +					 &(template->fields),
> +					 &(template->num_fields));

Hi Dmitry

ok, I'm fine with the change even if the template
initialization routine will be used for other purposes
(array items will be added in a linked list to permit
templates dynamic registration).

Thanks

Roberto Sassu


>   }
>

Re: [Linux-ima-devel] [PATCH 8/8] ima: initialize only required template

[Dmitry Kasatkin]

On 03/09/14 16:45, Roberto Sassu wrote:
> On 09/03/2014 09:20 AM, Dmitry Kasatkin wrote:
>> IMA uses only one template. This patch initializes only required
>> template to avoid unnecessary memory allocations.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> ---
>>   security/integrity/ima/ima_template.c | 28
>> ++++------------------------
>>   1 file changed, 4 insertions(+), 24 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_template.c
>> b/security/integrity/ima/ima_template.c
>> index f682606..e854862 100644
>> --- a/security/integrity/ima/ima_template.c
>> +++ b/security/integrity/ima/ima_template.c
>> @@ -152,24 +152,6 @@ out:
>>       return result;
>>   }
>>
>> -static int __init init_defined_templates(void)
>> -{
>> -    int i = 0;
>> -    int result = 0;
>> -
>> -    /* Init defined templates. */
>> -    for (i = 0; i < ARRAY_SIZE(defined_templates); i++) {
>> -        struct ima_template_desc *template = &defined_templates[i];
>> -
>> -        result = template_desc_init_fields(template->fmt,
>> -                           &(template->fields),
>> -                           &(template->num_fields));
>> -        if (result < 0)
>> -            return result;
>> -    }
>> -    return result;
>> -}
>> -
>>   struct ima_template_desc *ima_template_desc_current(void)
>>   {
>>       if (!ima_template)
>> @@ -180,11 +162,9 @@ struct ima_template_desc
>> *ima_template_desc_current(void)
>>
>>   int __init ima_init_template(void)
>>   {
>> -    int result;
>> -
>> -    result = init_defined_templates();
>> -    if (result < 0)
>> -        return result;
>> +    struct ima_template_desc *template = ima_template_desc_current();
>>
>> -    return 0;
>> +    return template_desc_init_fields(template->fmt,
>> +                     &(template->fields),
>> +                     &(template->num_fields));
>
> Hi Dmitry
>
> ok, I'm fine with the change even if the template
> initialization routine will be used for other purposes
> (array items will be added in a linked list to permit
> templates dynamic registration).
>
> Thanks
>
> Roberto Sassu
>

Hi Roberto,

Welcome back from holidays.

Sure, initialization function can be used later as well..

- Dmitry

>
>>   }
>>
>
>

Re: [Linux-ima-devel] [PATCH 5/8] ima: add missing '__init' keywords

[Roberto Sassu]

On 09/03/2014 09:19 AM, Dmitry Kasatkin wrote:
> Add missing keywords to the function definition to cleanup
> to discard initialization code.
>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>   security/integrity/ima/ima.h          | 2 --
>   security/integrity/ima/ima_crypto.c   | 2 +-
>   security/integrity/ima/ima_template.c | 4 ++--
>   3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c6990a7..8e4bb88 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -107,8 +107,6 @@ void ima_print_digest(struct seq_file *m, u8 *digest, int size);
>   struct ima_template_desc *ima_template_desc_current(void);
>   int ima_init_template(void);
>
> -int ima_init_template(void);
> -
>   /*
>    * used to protect h_table and sha_table
>    */
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 3b26472..d34e7df 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -97,7 +97,7 @@ static int ima_kernel_read(struct file *file, loff_t offset,
>   	return ret;
>   }
>
> -int ima_init_crypto(void)
> +int __init ima_init_crypto(void)
>   {
>   	long rc;
>
> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index a076a96..f682606 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -152,7 +152,7 @@ out:
>   	return result;
>   }
>
> -static int init_defined_templates(void)
> +static int __init init_defined_templates(void)
>   {
>   	int i = 0;
>   	int result = 0;
> @@ -178,7 +178,7 @@ struct ima_template_desc *ima_template_desc_current(void)
>   	return ima_template;
>   }
>
> -int ima_init_template(void)
> +int __init ima_init_template(void)
>   {
>   	int result;
>

Hi Dmitry

ok, I'm fine with this change.

Thanks

Roberto Sassu


>

Re: [PATCH 7/8] ima: remove usage of filename parameter

[Mimi Zohar]

On Wed, 2014-09-03 at 16:28 +0300, Dmitry Kasatkin wrote: 
> On 03/09/14 16:16, Mimi Zohar wrote:
> > On Wed, 2014-09-03 at 10:20 +0300, Dmitry Kasatkin wrote: 
> >> In all cases except ima_bprm_check() filename was not defined and
> >> ima_d_path() was used to find full path.
> >>
> >> ima_bprm_check() used to select between bprm->interp and bprm->filename.
> >> Following dump demonstrates differences between using filename and interp.
> >>
> >> bprm->filename
> >>  filename: ./foo.sh, pathname: /root/bin/foo.sh
> >>  filename: ./foo.sh, pathname: /bin/dash
> >>
> >> bprm->interp
> >>  filename: ./foo.sh, pathname: /root/bin/foo.sh
> >>  filename: /bin/sh, pathname: /bin/dash
> >>
> >> In both cases pathnames are the same.
> >> This patch removes usage of filename and interp in favor of d_path.
> >>
> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > Thanks, this has been on my list to do.  My only concern is whether we
> > should be using d_path() or one of the other variants (eg.
> > dentry_path(), d_absolute_path()).  For namespaces, we would want to be
> > able to differentiate the files.
> >
> > Please include in this patch description why d_path(), if it is the
> > case, the best option.
> >
> > thanks,
> 
> Hi,
> 
> Actually as we discussed, we can also in this patch change ima_d_path to
> use d_absolute_path().
> It will work for "chroot" cases and will show real path...
> 
> Should I switch to 'd_absolute_path'?

Yes, please.

> In the case of namespaces, neither d_path nor d_absolute_path works....
> Usage of dentry_path() would eliminate mount tree and requires device
> prefix.
> But it will 'break' clients, reading process measurement list.
> That would require essentially more agreement.

Right, we shouldn't break anything, but define a new template field for
the device or some other info.

Mimi