linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] ima: Provide more info about buffer measurement
@ 2021-07-01 12:55 Roberto Sassu
  2021-07-01 12:55 ` [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo() Roberto Sassu
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
  To: zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
	linux-integrity, linux-security-module, linux-kernel, selinux,
	Roberto Sassu

This patch set provides more information about buffer measurement.

First, it introduces the new function ima_get_current_hash_algo(), to
obtain the algorithm used to calculate the buffer digest (patch 1).

Second, it changes the type of return value of ima_measure_critical_data()
and process_buffer_measurement() from void to int, to signal to the callers
whether or not the buffer has been measured, or just the digest has been
calculated and written to the supplied location (patch 2).

Lastly, it adds two new parameters to the functions above ('digest' and
'digest_len'), so that those functions can write the buffer digest to the
location supplied by the callers (patch 3).

This patch set replaces the patch 'ima: Add digest, algo, measured
parameters to ima_measure_critical_data()' in:

https://lore.kernel.org/linux-integrity/20210625165614.2284243-1-roberto.sassu@huawei.com/

Changelog

v1:
- add digest_len parameter to ima_measure_critical_data() and
  process_buffer_measurement() (suggested by Lakshmi)
- fix doc formatting issues

Huawei Digest Lists patch set:
- introduce ima_get_current_hash_algo() (suggested by Mimi)
- remove algo and measured parameters from ima_measure_critical_data() and
  process_buffer_measurement() (suggested by Mimi)
- return an integer from ima_measure_critical_data() and
  process_buffer_measurement() (suggested by Mimi)
- correctly check when process_buffer_measurement() should return earlier

Roberto Sassu (3):
  ima: Introduce ima_get_current_hash_algo()
  ima: Return int in the functions to measure a buffer
  ima: Add digest and digest_len params to the functions to measure a
    buffer

 include/linux/ima.h                          | 23 ++++--
 security/integrity/ima/ima.h                 | 10 +--
 security/integrity/ima/ima_appraise.c        |  6 +-
 security/integrity/ima/ima_asymmetric_keys.c |  6 +-
 security/integrity/ima/ima_init.c            |  6 +-
 security/integrity/ima/ima_main.c            | 73 ++++++++++++++------
 security/integrity/ima/ima_queue_keys.c      | 15 ++--
 security/selinux/ima.c                       | 11 +--
 8 files changed, 100 insertions(+), 50 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo()
  2021-07-01 12:55 [PATCH v2 0/3] ima: Provide more info about buffer measurement Roberto Sassu
@ 2021-07-01 12:55 ` Roberto Sassu
  2021-07-01 16:01   ` Lakshmi Ramasubramanian
  2021-07-01 12:55 ` [PATCH v2 2/3] ima: Return int in the functions to measure a buffer Roberto Sassu
  2021-07-01 12:55 ` [PATCH v2 3/3] ima: Add digest and digest_len params to " Roberto Sassu
  2 siblings, 1 reply; 9+ messages in thread
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
  To: zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
	linux-integrity, linux-security-module, linux-kernel, selinux,
	Roberto Sassu

This patch introduces the new function ima_get_current_hash_algo(), that
callers in the other kernel subsystems might use to obtain the hash
algorithm selected by IMA.

Its primary use will be to determine which algorithm has been used to
calculate the digest written by ima_measure_critical_data() to the location
passed as a new parameter (in a subsequent patch).

Since the hash algorithm does not change after the IMA setup phase, there
is no risk of races (obtaining a digest calculated with a different
algorithm than the one returned).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/ima.h               | 7 +++++++
 security/integrity/ima/ima_main.c | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 61d5723ec303..81e830d01ced 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,9 +11,11 @@
 #include <linux/fs.h>
 #include <linux/security.h>
 #include <linux/kexec.h>
+#include <crypto/hash_info.h>
 struct linux_binprm;
 
 #ifdef CONFIG_IMA
+extern enum hash_algo ima_get_current_hash_algo(void);
 extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask);
 extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
@@ -64,6 +66,11 @@ static inline const char * const *arch_get_ima_policy(void)
 #endif
 
 #else
+static inline enum hash_algo ima_get_current_hash_algo(void)
+{
+	return HASH_ALGO__LAST;
+}
+
 static inline int ima_bprm_check(struct linux_binprm *bprm)
 {
 	return 0;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 287b90509006..8ef1fa357e0c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -76,6 +76,11 @@ static int __init hash_setup(char *str)
 }
 __setup("ima_hash=", hash_setup);
 
+enum hash_algo ima_get_current_hash_algo(void)
+{
+	return ima_hash_algo;
+}
+
 /* Prevent mmap'ing a file execute that is already mmap'ed write */
 static int mmap_violation_check(enum ima_hooks func, struct file *file,
 				char **pathbuf, const char **pathname,
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] ima: Return int in the functions to measure a buffer
  2021-07-01 12:55 [PATCH v2 0/3] ima: Provide more info about buffer measurement Roberto Sassu
  2021-07-01 12:55 ` [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo() Roberto Sassu
@ 2021-07-01 12:55 ` Roberto Sassu
  2021-07-01 16:15   ` Lakshmi Ramasubramanian
  2021-07-01 12:55 ` [PATCH v2 3/3] ima: Add digest and digest_len params to " Roberto Sassu
  2 siblings, 1 reply; 9+ messages in thread
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
  To: zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
	linux-integrity, linux-security-module, linux-kernel, selinux,
	Roberto Sassu

ima_measure_critical_data() and process_buffer_measurement() currently
don't return a result. A caller wouldn't be able to know whether those
functions were executed successfully.

This patch modifies the return type from void to int, and returns 0 if the
buffer has been successfully measured, a negative value otherwise.

Also, this patch does not modify the behavior of existing callers by
processing the returned value. Instead, the value is stored in a variable
marked as __maybe_unused.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/ima.h                          | 15 +++---
 security/integrity/ima/ima.h                 | 10 ++--
 security/integrity/ima/ima_appraise.c        |  4 +-
 security/integrity/ima/ima_asymmetric_keys.c |  4 +-
 security/integrity/ima/ima_init.c            |  6 ++-
 security/integrity/ima/ima_main.c            | 48 ++++++++++++--------
 security/integrity/ima/ima_queue_keys.c      | 15 +++---
 security/selinux/ima.c                       | 10 ++--
 8 files changed, 66 insertions(+), 46 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 81e830d01ced..60492263aa64 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
 extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
 extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
 extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
-extern void ima_measure_critical_data(const char *event_label,
-				      const char *event_name,
-				      const void *buf, size_t buf_len,
-				      bool hash);
+extern int ima_measure_critical_data(const char *event_label,
+				     const char *event_name,
+				     const void *buf, size_t buf_len,
+				     bool hash);
 
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
 extern void ima_appraise_parse_cmdline(void);
@@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size
 
 static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
 
-static inline void ima_measure_critical_data(const char *event_label,
+static inline int ima_measure_critical_data(const char *event_label,
 					     const char *event_name,
 					     const void *buf, size_t buf_len,
-					     bool hash) {}
+					     bool hash)
+{
+	return -ENOENT;
+}
 
 #endif /* CONFIG_IMA */
 
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f0e448ed1f9f..03db221324c3 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -264,11 +264,11 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, const struct modsig *modsig, int pcr,
 			   struct ima_template_desc *template_desc);
-void process_buffer_measurement(struct user_namespace *mnt_userns,
-				struct inode *inode, const void *buf, int size,
-				const char *eventname, enum ima_hooks func,
-				int pcr, const char *func_data,
-				bool buf_hash);
+int process_buffer_measurement(struct user_namespace *mnt_userns,
+			       struct inode *inode, const void *buf, int size,
+			       const char *eventname, enum ima_hooks func,
+			       int pcr, const char *func_data,
+			       bool buf_hash);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..275a2377743f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -345,6 +345,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
 	enum hash_algo hash_algo;
 	const u8 *digest = NULL;
 	u32 digestsize = 0;
+	int process_rc __maybe_unused;
 	int rc = 0;
 
 	if (!(iint->flags & IMA_CHECK_BLACKLIST))
@@ -355,7 +356,8 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
 
 		rc = is_binary_blacklisted(digest, digestsize);
 		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
-			process_buffer_measurement(&init_user_ns, NULL, digest, digestsize,
+			process_rc = process_buffer_measurement(&init_user_ns,
+						   NULL, digest, digestsize,
 						   "blacklisted-hash", NONE,
 						   pcr, NULL, false);
 	}
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index c985418698a4..910367cdd920 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -31,6 +31,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 				   unsigned long flags, bool create)
 {
 	bool queued = false;
+	int ret __maybe_unused;
 
 	/* Only asymmetric keys are handled by this hook. */
 	if (key->type != &key_type_asymmetric)
@@ -60,7 +61,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 	 * if the IMA policy is configured to measure a key linked
 	 * to the given keyring.
 	 */
-	process_buffer_measurement(&init_user_ns, NULL, payload, payload_len,
+	ret = process_buffer_measurement(&init_user_ns, NULL,
+				   payload, payload_len,
 				   keyring->description, KEY_CHECK, 0,
 				   keyring->description, false);
 }
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5076a7d9d23e..6790eea88db8 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -118,6 +118,7 @@ void __init ima_load_x509(void)
 
 int __init ima_init(void)
 {
+	int measure_rc __maybe_unused;
 	int rc;
 
 	ima_tpm_chip = tpm_default_chip();
@@ -153,8 +154,9 @@ int __init ima_init(void)
 
 	ima_init_key_queue();
 
-	ima_measure_critical_data("kernel_info", "kernel_version",
-				  UTS_RELEASE, strlen(UTS_RELEASE), false);
+	measure_rc = ima_measure_critical_data("kernel_info", "kernel_version",
+					       UTS_RELEASE, strlen(UTS_RELEASE),
+					       false);
 
 	return rc;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8ef1fa357e0c..3386e7436440 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size,
 	return 0;
 }
 
-/*
+/**
  * process_buffer_measurement - Measure the buffer or the buffer data hash
  * @mnt_userns:	user namespace of the mount the inode was found from
  * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
@@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size,
  * @buf_hash: measure buffer data hash
  *
  * Based on policy, either the buffer data or buffer data hash is measured
+ *
+ * Return: 0 if the buffer has been successfully measured, a negative value
+ * otherwise.
  */
-void process_buffer_measurement(struct user_namespace *mnt_userns,
-				struct inode *inode, const void *buf, int size,
-				const char *eventname, enum ima_hooks func,
-				int pcr, const char *func_data,
-				bool buf_hash)
+int process_buffer_measurement(struct user_namespace *mnt_userns,
+			       struct inode *inode, const void *buf, int size,
+			       const char *eventname, enum ima_hooks func,
+			       int pcr, const char *func_data,
+			       bool buf_hash)
 {
 	int ret = 0;
 	const char *audit_cause = "ENOMEM";
@@ -867,7 +870,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
 	u32 secid;
 
 	if (!ima_policy_flag)
-		return;
+		return -ENOENT;
 
 	template = ima_template_desc_buf();
 	if (!template) {
@@ -889,7 +892,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
 					secid, 0, func, &pcr, &template,
 					func_data);
 		if (!(action & IMA_MEASURE))
-			return;
+			return -ENOENT;
 	}
 
 	if (!pcr)
@@ -937,7 +940,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
 					func_measure_str(func),
 					audit_cause, ret, 0, ret);
 
-	return;
+	return ret;
 }
 
 /**
@@ -951,6 +954,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
 void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 {
 	struct fd f;
+	int ret __maybe_unused;
 
 	if (!buf || !size)
 		return;
@@ -959,9 +963,10 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 	if (!f.file)
 		return;
 
-	process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file),
-				   buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
-				   NULL, false);
+	ret = process_buffer_measurement(file_mnt_user_ns(f.file),
+					 file_inode(f.file), buf, size,
+					 "kexec-cmdline", KEXEC_CMDLINE, 0,
+					 NULL, false);
 	fdput(f);
 }
 
@@ -977,18 +982,21 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
  * and extend the pcr.  Examples of critical data could be various data
  * structures, policies, and states stored in kernel memory that can
  * impact the integrity of the system.
+ *
+ * Return: 0 if the buffer has been successfully measured, a negative value
+ * otherwise.
  */
-void ima_measure_critical_data(const char *event_label,
-			       const char *event_name,
-			       const void *buf, size_t buf_len,
-			       bool hash)
+int ima_measure_critical_data(const char *event_label,
+			      const char *event_name,
+			      const void *buf, size_t buf_len,
+			      bool hash)
 {
 	if (!event_name || !event_label || !buf || !buf_len)
-		return;
+		return -ENOPARAM;
 
-	process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name,
-				   CRITICAL_DATA, 0, event_label,
-				   hash);
+	return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
+					  event_name, CRITICAL_DATA, 0,
+					  event_label, hash);
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 979ef6c71f3d..e3047ce64f39 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -134,6 +134,7 @@ void ima_process_queued_keys(void)
 {
 	struct ima_key_entry *entry, *tmp;
 	bool process = false;
+	int ret __maybe_unused;
 
 	if (ima_process_keys)
 		return;
@@ -159,13 +160,13 @@ void ima_process_queued_keys(void)
 
 	list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
 		if (!timer_expired)
-			process_buffer_measurement(&init_user_ns, NULL,
-						   entry->payload,
-						   entry->payload_len,
-						   entry->keyring_name,
-						   KEY_CHECK, 0,
-						   entry->keyring_name,
-						   false);
+			ret = process_buffer_measurement(&init_user_ns, NULL,
+							 entry->payload,
+							 entry->payload_len,
+							 entry->keyring_name,
+							 KEY_CHECK, 0,
+							 entry->keyring_name,
+							 false);
 		list_del(&entry->list);
 		ima_free_key_entry(entry);
 	}
diff --git a/security/selinux/ima.c b/security/selinux/ima.c
index 34d421861bfc..4db9fa211638 100644
--- a/security/selinux/ima.c
+++ b/security/selinux/ima.c
@@ -75,6 +75,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
 	char *state_str = NULL;
 	void *policy = NULL;
 	size_t policy_len;
+	int measure_rc __maybe_unused;
 	int rc = 0;
 
 	WARN_ON(!mutex_is_locked(&state->policy_mutex));
@@ -85,8 +86,9 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
 		return;
 	}
 
-	ima_measure_critical_data("selinux", "selinux-state",
-				  state_str, strlen(state_str), false);
+	measure_rc = ima_measure_critical_data("selinux", "selinux-state",
+					       state_str, strlen(state_str),
+					       false);
 
 	kfree(state_str);
 
@@ -102,8 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
 		return;
 	}
 
-	ima_measure_critical_data("selinux", "selinux-policy-hash",
-				  policy, policy_len, true);
+	measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
+					       policy, policy_len, true);
 
 	vfree(policy);
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
  2021-07-01 12:55 [PATCH v2 0/3] ima: Provide more info about buffer measurement Roberto Sassu
  2021-07-01 12:55 ` [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo() Roberto Sassu
  2021-07-01 12:55 ` [PATCH v2 2/3] ima: Return int in the functions to measure a buffer Roberto Sassu
@ 2021-07-01 12:55 ` Roberto Sassu
  2021-07-01 17:27   ` Lakshmi Ramasubramanian
  2 siblings, 1 reply; 9+ messages in thread
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
  To: zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
	linux-integrity, linux-security-module, linux-kernel, selinux,
	Roberto Sassu

This patch adds the 'digest' and 'digest_len' parameters to
ima_measure_critical_data() and process_buffer_measurement(), so that
callers can get the digest of the passed buffer.

These functions calculate the digest even if there is no suitable rule in
the IMA policy and, in this case, they simply return 1 before generating a
new measurement entry.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/ima.h                          |  5 +--
 security/integrity/ima/ima.h                 |  2 +-
 security/integrity/ima/ima_appraise.c        |  2 +-
 security/integrity/ima/ima_asymmetric_keys.c |  2 +-
 security/integrity/ima/ima_init.c            |  2 +-
 security/integrity/ima/ima_main.c            | 36 ++++++++++++++------
 security/integrity/ima/ima_queue_keys.c      |  2 +-
 security/selinux/ima.c                       |  5 +--
 8 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 60492263aa64..b6ab66a546ae 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -38,7 +38,7 @@ extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
 extern int ima_measure_critical_data(const char *event_label,
 				     const char *event_name,
 				     const void *buf, size_t buf_len,
-				     bool hash);
+				     bool hash, u8 *digest, size_t digest_len);
 
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
 extern void ima_appraise_parse_cmdline(void);
@@ -147,7 +147,8 @@ static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {
 static inline int ima_measure_critical_data(const char *event_label,
 					     const char *event_name,
 					     const void *buf, size_t buf_len,
-					     bool hash)
+					     bool hash, u8 *digest,
+					     size_t digest_len)
 {
 	return -ENOENT;
 }
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 03db221324c3..2f4c20b16ad7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -268,7 +268,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
 			       struct inode *inode, const void *buf, int size,
 			       const char *eventname, enum ima_hooks func,
 			       int pcr, const char *func_data,
-			       bool buf_hash);
+			       bool buf_hash, u8 *digest, size_t digest_len);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 275a2377743f..6ac8715f3563 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -359,7 +359,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
 			process_rc = process_buffer_measurement(&init_user_ns,
 						   NULL, digest, digestsize,
 						   "blacklisted-hash", NONE,
-						   pcr, NULL, false);
+						   pcr, NULL, false, NULL, 0);
 	}
 
 	return rc;
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 910367cdd920..0ad9995fab98 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -64,5 +64,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 	ret = process_buffer_measurement(&init_user_ns, NULL,
 				   payload, payload_len,
 				   keyring->description, KEY_CHECK, 0,
-				   keyring->description, false);
+				   keyring->description, false, NULL, 0);
 }
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 6790eea88db8..4f5708d68cc7 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -156,7 +156,7 @@ int __init ima_init(void)
 
 	measure_rc = ima_measure_critical_data("kernel_info", "kernel_version",
 					       UTS_RELEASE, strlen(UTS_RELEASE),
-					       false);
+					       false, NULL, 0);
 
 	return rc;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3386e7436440..b4b1dc25e4fb 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size,
  * @pcr: pcr to extend the measurement
  * @func_data: func specific data, may be NULL
  * @buf_hash: measure buffer data hash
+ * @digest: buffer digest will be written to
+ * @digest_len: buffer length
  *
  * Based on policy, either the buffer data or buffer data hash is measured
  *
- * Return: 0 if the buffer has been successfully measured, a negative value
- * otherwise.
+ * Return: 0 if the buffer has been successfully measured, 1 if the digest
+ * has been written to the passed location but not added to a measurement entry,
+ * a negative value otherwise.
  */
 int process_buffer_measurement(struct user_namespace *mnt_userns,
 			       struct inode *inode, const void *buf, int size,
 			       const char *eventname, enum ima_hooks func,
 			       int pcr, const char *func_data,
-			       bool buf_hash)
+			       bool buf_hash, u8 *digest, size_t digest_len)
 {
 	int ret = 0;
 	const char *audit_cause = "ENOMEM";
@@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
 	int action = 0;
 	u32 secid;
 
-	if (!ima_policy_flag)
+	if (digest && digest_len < digest_hash_len)
+		return -EINVAL;
+
+	if (!ima_policy_flag && !digest)
 		return -ENOENT;
 
 	template = ima_template_desc_buf();
@@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
 		action = ima_get_action(mnt_userns, inode, current_cred(),
 					secid, 0, func, &pcr, &template,
 					func_data);
-		if (!(action & IMA_MEASURE))
+		if (!(action & IMA_MEASURE) && !digest)
 			return -ENOENT;
 	}
 
@@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
 		event_data.buf_len = digest_hash_len;
 	}
 
+	if (digest)
+		memcpy(digest, iint.ima_hash->digest, digest_hash_len);
+
+	if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
+		return 1;
+
 	ret = ima_alloc_init_template(&event_data, &entry, template);
 	if (ret < 0) {
 		audit_cause = "alloc_entry";
@@ -966,7 +978,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 	ret = process_buffer_measurement(file_mnt_user_ns(f.file),
 					 file_inode(f.file), buf, size,
 					 "kexec-cmdline", KEXEC_CMDLINE, 0,
-					 NULL, false);
+					 NULL, false, NULL, 0);
 	fdput(f);
 }
 
@@ -977,26 +989,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
  * @buf: pointer to buffer data
  * @buf_len: length of buffer data (in bytes)
  * @hash: measure buffer data hash
+ * @digest: buffer digest will be written to
+ * @digest_len: buffer length
  *
  * Measure data critical to the integrity of the kernel into the IMA log
  * and extend the pcr.  Examples of critical data could be various data
  * structures, policies, and states stored in kernel memory that can
  * impact the integrity of the system.
  *
- * Return: 0 if the buffer has been successfully measured, a negative value
- * otherwise.
+ * Return: 0 if the buffer has been successfully measured, 1 if the digest
+ * has been written to the passed location but not added to a measurement entry,
+ * a negative value otherwise.
  */
 int ima_measure_critical_data(const char *event_label,
 			      const char *event_name,
 			      const void *buf, size_t buf_len,
-			      bool hash)
+			      bool hash, u8 *digest, size_t digest_len)
 {
 	if (!event_name || !event_label || !buf || !buf_len)
 		return -ENOPARAM;
 
 	return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
 					  event_name, CRITICAL_DATA, 0,
-					  event_label, hash);
+					  event_label, hash, digest,
+					  digest_len);
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index e3047ce64f39..b02b061c5fac 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -166,7 +166,7 @@ void ima_process_queued_keys(void)
 							 entry->keyring_name,
 							 KEY_CHECK, 0,
 							 entry->keyring_name,
-							 false);
+							 false, NULL, 0);
 		list_del(&entry->list);
 		ima_free_key_entry(entry);
 	}
diff --git a/security/selinux/ima.c b/security/selinux/ima.c
index 4db9fa211638..d5d7b3ca9651 100644
--- a/security/selinux/ima.c
+++ b/security/selinux/ima.c
@@ -88,7 +88,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
 
 	measure_rc = ima_measure_critical_data("selinux", "selinux-state",
 					       state_str, strlen(state_str),
-					       false);
+					       false, NULL, 0);
 
 	kfree(state_str);
 
@@ -105,7 +105,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
 	}
 
 	measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
-					       policy, policy_len, true);
+					       policy, policy_len, true,
+					       NULL, 0);
 
 	vfree(policy);
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo()
  2021-07-01 12:55 ` [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo() Roberto Sassu
@ 2021-07-01 16:01   ` Lakshmi Ramasubramanian
  0 siblings, 0 replies; 9+ messages in thread
From: Lakshmi Ramasubramanian @ 2021-07-01 16:01 UTC (permalink / raw)
  To: Roberto Sassu, zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
	linux-security-module, linux-kernel, selinux

On 7/1/2021 5:55 AM, Roberto Sassu wrote:
> This patch introduces the new function ima_get_current_hash_algo(), that
> callers in the other kernel subsystems might use to obtain the hash
> algorithm selected by IMA.
> 
> Its primary use will be to determine which algorithm has been used to
> calculate the digest written by ima_measure_critical_data() to the location
> passed as a new parameter (in a subsequent patch). >
> Since the hash algorithm does not change after the IMA setup phase, there
> is no risk of races (obtaining a digest calculated with a different
> algorithm than the one returned).

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

  -lakshmi

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   include/linux/ima.h               | 7 +++++++
>   security/integrity/ima/ima_main.c | 5 +++++
>   2 files changed, 12 insertions(+)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 61d5723ec303..81e830d01ced 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -11,9 +11,11 @@
>   #include <linux/fs.h>
>   #include <linux/security.h>
>   #include <linux/kexec.h>
> +#include <crypto/hash_info.h>
>   struct linux_binprm;
>   
>   #ifdef CONFIG_IMA
> +extern enum hash_algo ima_get_current_hash_algo(void);
>   extern int ima_bprm_check(struct linux_binprm *bprm);
>   extern int ima_file_check(struct file *file, int mask);
>   extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
> @@ -64,6 +66,11 @@ static inline const char * const *arch_get_ima_policy(void)
>   #endif
>   
>   #else
> +static inline enum hash_algo ima_get_current_hash_algo(void)
> +{
> +	return HASH_ALGO__LAST;
> +}
> +
>   static inline int ima_bprm_check(struct linux_binprm *bprm)
>   {
>   	return 0;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 287b90509006..8ef1fa357e0c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -76,6 +76,11 @@ static int __init hash_setup(char *str)
>   }
>   __setup("ima_hash=", hash_setup);
>   
> +enum hash_algo ima_get_current_hash_algo(void)
> +{
> +	return ima_hash_algo;
> +}
> +
>   /* Prevent mmap'ing a file execute that is already mmap'ed write */
>   static int mmap_violation_check(enum ima_hooks func, struct file *file,
>   				char **pathbuf, const char **pathname,
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] ima: Return int in the functions to measure a buffer
  2021-07-01 12:55 ` [PATCH v2 2/3] ima: Return int in the functions to measure a buffer Roberto Sassu
@ 2021-07-01 16:15   ` Lakshmi Ramasubramanian
  2021-07-02 10:51     ` Roberto Sassu
  0 siblings, 1 reply; 9+ messages in thread
From: Lakshmi Ramasubramanian @ 2021-07-01 16:15 UTC (permalink / raw)
  To: Roberto Sassu, zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
	linux-security-module, linux-kernel, selinux

On 7/1/2021 5:55 AM, Roberto Sassu wrote:
> ima_measure_critical_data() and process_buffer_measurement() currently
> don't return a result. A caller wouldn't be able to know whether those
> functions were executed successfully.
> 
> This patch modifies the return type from void to int, and returns 0 if the
> buffer has been successfully measured, a negative value otherwise.
> 
> Also, this patch does not modify the behavior of existing callers by
> processing the returned value. Instead, the value is stored in a variable
> marked as __maybe_unused.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   include/linux/ima.h                          | 15 +++---
>   security/integrity/ima/ima.h                 | 10 ++--
>   security/integrity/ima/ima_appraise.c        |  4 +-
>   security/integrity/ima/ima_asymmetric_keys.c |  4 +-
>   security/integrity/ima/ima_init.c            |  6 ++-
>   security/integrity/ima/ima_main.c            | 48 ++++++++++++--------
>   security/integrity/ima/ima_queue_keys.c      | 15 +++---
>   security/selinux/ima.c                       | 10 ++--
>   8 files changed, 66 insertions(+), 46 deletions(-)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 81e830d01ced..60492263aa64 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
>   extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
>   extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
>   extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> -extern void ima_measure_critical_data(const char *event_label,
> -				      const char *event_name,
> -				      const void *buf, size_t buf_len,
> -				      bool hash);
> +extern int ima_measure_critical_data(const char *event_label,
> +				     const char *event_name,
> +				     const void *buf, size_t buf_len,
> +				     bool hash);
>   
>   #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>   extern void ima_appraise_parse_cmdline(void);
> @@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size
>   
>   static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
>   
> -static inline void ima_measure_critical_data(const char *event_label,
> +static inline int ima_measure_critical_data(const char *event_label,
>   					     const char *event_name,
>   					     const void *buf, size_t buf_len,
> -					     bool hash) {}
> +					     bool hash)
> +{
> +	return -ENOENT;
> +}
>   
>   #endif /* CONFIG_IMA */
>   
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index f0e448ed1f9f..03db221324c3 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -264,11 +264,11 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
>   			   struct evm_ima_xattr_data *xattr_value,
>   			   int xattr_len, const struct modsig *modsig, int pcr,
>   			   struct ima_template_desc *template_desc);
> -void process_buffer_measurement(struct user_namespace *mnt_userns,
> -				struct inode *inode, const void *buf, int size,
> -				const char *eventname, enum ima_hooks func,
> -				int pcr, const char *func_data,
> -				bool buf_hash);
> +int process_buffer_measurement(struct user_namespace *mnt_userns,
> +			       struct inode *inode, const void *buf, int size,
> +			       const char *eventname, enum ima_hooks func,
> +			       int pcr, const char *func_data,
> +			       bool buf_hash);
>   void ima_audit_measurement(struct integrity_iint_cache *iint,
>   			   const unsigned char *filename);
>   int ima_alloc_init_template(struct ima_event_data *event_data,
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ef9dcfce45d4..275a2377743f 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -345,6 +345,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
>   	enum hash_algo hash_algo;
>   	const u8 *digest = NULL;
>   	u32 digestsize = 0;
> +	int process_rc __maybe_unused;
>   	int rc = 0;
>   
>   	if (!(iint->flags & IMA_CHECK_BLACKLIST))
> @@ -355,7 +356,8 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
>   
>   		rc = is_binary_blacklisted(digest, digestsize);
>   		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> -			process_buffer_measurement(&init_user_ns, NULL, digest, digestsize,
> +			process_rc = process_buffer_measurement(&init_user_ns,
I think there is no need to make this change now. If and when 
ima_check_blacklist() needs to look at the return value of p_b_m(), this 
change can be made.

> +						   NULL, digest, digestsize,
>   						   "blacklisted-hash", NONE,
>   						   pcr, NULL, false);
>   	}
> diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
> index c985418698a4..910367cdd920 100644
> --- a/security/integrity/ima/ima_asymmetric_keys.c
> +++ b/security/integrity/ima/ima_asymmetric_keys.c
> @@ -31,6 +31,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>   				   unsigned long flags, bool create)
>   {
>   	bool queued = false;
> +	int ret __maybe_unused;
>   
>   	/* Only asymmetric keys are handled by this hook. */
>   	if (key->type != &key_type_asymmetric)
> @@ -60,7 +61,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>   	 * if the IMA policy is configured to measure a key linked
>   	 * to the given keyring.
>   	 */
> -	process_buffer_measurement(&init_user_ns, NULL, payload, payload_len,
> +	ret = process_buffer_measurement(&init_user_ns, NULL,
> +				   payload, payload_len,
Same comment as in ima_check_blacklist() - this change be made when needed.

>   				   keyring->description, KEY_CHECK, 0,
>   				   keyring->description, false);
>   }
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 5076a7d9d23e..6790eea88db8 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -118,6 +118,7 @@ void __init ima_load_x509(void)
>   
>   int __init ima_init(void)
>   {
> +	int measure_rc __maybe_unused;
>   	int rc;
>   
>   	ima_tpm_chip = tpm_default_chip();
> @@ -153,8 +154,9 @@ int __init ima_init(void)
>   
>   	ima_init_key_queue();
>   
> -	ima_measure_critical_data("kernel_info", "kernel_version",
> -				  UTS_RELEASE, strlen(UTS_RELEASE), false);
> +	measure_rc = ima_measure_critical_data("kernel_info", "kernel_version",
> +					       UTS_RELEASE, strlen(UTS_RELEASE),
> +					       false);
Same comment as in ima_check_blacklist() - this change be made when needed.

>   
>   	return rc;
>   }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 8ef1fa357e0c..3386e7436440 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size,
>   	return 0;
>   }
>   
> -/*
> +/**
>    * process_buffer_measurement - Measure the buffer or the buffer data hash
>    * @mnt_userns:	user namespace of the mount the inode was found from
>    * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
> @@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size,
>    * @buf_hash: measure buffer data hash
>    *
>    * Based on policy, either the buffer data or buffer data hash is measured
> + *
> + * Return: 0 if the buffer has been successfully measured, a negative value
> + * otherwise.
>    */
> -void process_buffer_measurement(struct user_namespace *mnt_userns,
> -				struct inode *inode, const void *buf, int size,
> -				const char *eventname, enum ima_hooks func,
> -				int pcr, const char *func_data,
> -				bool buf_hash)
> +int process_buffer_measurement(struct user_namespace *mnt_userns,
> +			       struct inode *inode, const void *buf, int size,
> +			       const char *eventname, enum ima_hooks func,
> +			       int pcr, const char *func_data,
> +			       bool buf_hash)
>   {
>   	int ret = 0;
>   	const char *audit_cause = "ENOMEM";
> @@ -867,7 +870,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
>   	u32 secid;
>   
>   	if (!ima_policy_flag)
> -		return;
> +		return -ENOENT;
>   
>   	template = ima_template_desc_buf();
>   	if (!template) {
> @@ -889,7 +892,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
>   					secid, 0, func, &pcr, &template,
>   					func_data);
>   		if (!(action & IMA_MEASURE))
> -			return;
> +			return -ENOENT;
>   	}
>   
>   	if (!pcr)
> @@ -937,7 +940,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
>   					func_measure_str(func),
>   					audit_cause, ret, 0, ret);
>   
> -	return;
> +	return ret;
>   }
>   
>   /**
> @@ -951,6 +954,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
>   void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>   {
>   	struct fd f;
> +	int ret __maybe_unused;
>   
>   	if (!buf || !size)
>   		return;
> @@ -959,9 +963,10 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>   	if (!f.file)
>   		return;
>   
> -	process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file),
> -				   buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
> -				   NULL, false);
> +	ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> +					 file_inode(f.file), buf, size,
> +					 "kexec-cmdline", KEXEC_CMDLINE, 0,
> +					 NULL, false);
Since the return value of p_b_m() is not used here, this change can be 
made when needed.

>   	fdput(f);
>   }
>   
> @@ -977,18 +982,21 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>    * and extend the pcr.  Examples of critical data could be various data
>    * structures, policies, and states stored in kernel memory that can
>    * impact the integrity of the system.
> + *
> + * Return: 0 if the buffer has been successfully measured, a negative value
> + * otherwise.
>    */
> -void ima_measure_critical_data(const char *event_label,
> -			       const char *event_name,
> -			       const void *buf, size_t buf_len,
> -			       bool hash)
> +int ima_measure_critical_data(const char *event_label,
> +			      const char *event_name,
> +			      const void *buf, size_t buf_len,
> +			      bool hash)
>   {
>   	if (!event_name || !event_label || !buf || !buf_len)
> -		return;
> +		return -ENOPARAM;
>   
> -	process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name,
> -				   CRITICAL_DATA, 0, event_label,
> -				   hash);
> +	return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
> +					  event_name, CRITICAL_DATA, 0,
> +					  event_label, hash);
>   }
>   
>   static int __init init_ima(void)
> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> index 979ef6c71f3d..e3047ce64f39 100644
> --- a/security/integrity/ima/ima_queue_keys.c
> +++ b/security/integrity/ima/ima_queue_keys.c
> @@ -134,6 +134,7 @@ void ima_process_queued_keys(void)
>   {
>   	struct ima_key_entry *entry, *tmp;
>   	bool process = false;
> +	int ret __maybe_unused;
>   
>   	if (ima_process_keys)
>   		return;
> @@ -159,13 +160,13 @@ void ima_process_queued_keys(void)
>   
>   	list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
>   		if (!timer_expired)
> -			process_buffer_measurement(&init_user_ns, NULL,
> -						   entry->payload,
> -						   entry->payload_len,
> -						   entry->keyring_name,
> -						   KEY_CHECK, 0,
> -						   entry->keyring_name,
> -						   false);
> +			ret = process_buffer_measurement(&init_user_ns, NULL,
> +							 entry->payload,
> +							 entry->payload_len,
> +							 entry->keyring_name,
> +							 KEY_CHECK, 0,
> +							 entry->keyring_name,
> +							 false);
Same comment as above.

>   		list_del(&entry->list);
>   		ima_free_key_entry(entry);
>   	}
> diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> index 34d421861bfc..4db9fa211638 100644
> --- a/security/selinux/ima.c
> +++ b/security/selinux/ima.c
> @@ -75,6 +75,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
>   	char *state_str = NULL;
>   	void *policy = NULL;
>   	size_t policy_len;
> +	int measure_rc __maybe_unused;
>   	int rc = 0;
>   
>   	WARN_ON(!mutex_is_locked(&state->policy_mutex));
> @@ -85,8 +86,9 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
>   		return;
>   	}
>   
> -	ima_measure_critical_data("selinux", "selinux-state",
> -				  state_str, strlen(state_str), false);
> +	measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> +					       state_str, strlen(state_str),
> +					       false);
Since the return value of ima_measure_critical_data() is not used here, 
this change can be made when needed.

>   
>   	kfree(state_str);
>   
> @@ -102,8 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
>   		return;
>   	}
>   
> -	ima_measure_critical_data("selinux", "selinux-policy-hash",
> -				  policy, policy_len, true);
> +	measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
> +					       policy, policy_len, true);
Same comment as above.

  -lakshmi

>   
>   	vfree(policy);
>   }
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
  2021-07-01 12:55 ` [PATCH v2 3/3] ima: Add digest and digest_len params to " Roberto Sassu
@ 2021-07-01 17:27   ` Lakshmi Ramasubramanian
  2021-07-02 10:54     ` Roberto Sassu
  0 siblings, 1 reply; 9+ messages in thread
From: Lakshmi Ramasubramanian @ 2021-07-01 17:27 UTC (permalink / raw)
  To: Roberto Sassu, zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
	linux-security-module, linux-kernel, selinux

On 7/1/2021 5:55 AM, Roberto Sassu wrote:

Hi Roberto,

> This patch adds the 'digest' and 'digest_len' parameters to
> ima_measure_critical_data() and process_buffer_measurement(), so that
> callers can get the digest of the passed buffer.
> 
> These functions calculate the digest even if there is no suitable rule in
> the IMA policy and, in this case, they simply return 1 before generating a
> new measurement entry.
> 

> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 3386e7436440..b4b1dc25e4fb 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size,
>    * @pcr: pcr to extend the measurement
>    * @func_data: func specific data, may be NULL
>    * @buf_hash: measure buffer data hash
> + * @digest: buffer digest will be written to
> + * @digest_len: buffer length
>    *
>    * Based on policy, either the buffer data or buffer data hash is measured
>    *
> - * Return: 0 if the buffer has been successfully measured, a negative value
> - * otherwise.
> + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> + * has been written to the passed location but not added to a measurement entry,
> + * a negative value otherwise.
>    */
>   int process_buffer_measurement(struct user_namespace *mnt_userns,
>   			       struct inode *inode, const void *buf, int size,
>   			       const char *eventname, enum ima_hooks func,
>   			       int pcr, const char *func_data,
> -			       bool buf_hash)
> +			       bool buf_hash, u8 *digest, size_t digest_len)
>   {
>   	int ret = 0;
>   	const char *audit_cause = "ENOMEM";
> @@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
>   	int action = 0;
>   	u32 secid;
>   
> -	if (!ima_policy_flag)
> +	if (digest && digest_len < digest_hash_len)
> +		return -EINVAL;
> +
> +	if (!ima_policy_flag && !digest)
>   		return -ENOENT;

Just wanted to check if you have verified this scenario:

If ima_policy_flag is 0, the in-memory ima policy data is not yet 
initialized. In this case calling ima_get_action() will cause kernel 
panic (NULL exception).

Please verify the above issue doesn't exist if the caller passes 
non-NULL digest and ima_policy_flag is 0 (ima policy is not initialized).

thanks,
  -lakshmi

>   
>   	template = ima_template_desc_buf();
> @@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
>   		action = ima_get_action(mnt_userns, inode, current_cred(),
>   					secid, 0, func, &pcr, &template,
>   					func_data);
> -		if (!(action & IMA_MEASURE))
> +		if (!(action & IMA_MEASURE) && !digest)
>   			return -ENOENT;
>   	}
>   
> @@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
>   		event_data.buf_len = digest_hash_len;
>   	}
>   
> +	if (digest)
> +		memcpy(digest, iint.ima_hash->digest, digest_hash_len);
> +
> +	if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
> +		return 1;
> +
>   	ret = ima_alloc_init_template(&event_data, &entry, template);
>   	if (ret < 0) {
>   		audit_cause = "alloc_entry";
> @@ -966,7 +978,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>   	ret = process_buffer_measurement(file_mnt_user_ns(f.file),
>   					 file_inode(f.file), buf, size,
>   					 "kexec-cmdline", KEXEC_CMDLINE, 0,
> -					 NULL, false);
> +					 NULL, false, NULL, 0);
>   	fdput(f);
>   }
>   
> @@ -977,26 +989,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>    * @buf: pointer to buffer data
>    * @buf_len: length of buffer data (in bytes)
>    * @hash: measure buffer data hash
> + * @digest: buffer digest will be written to
> + * @digest_len: buffer length
>    *
>    * Measure data critical to the integrity of the kernel into the IMA log
>    * and extend the pcr.  Examples of critical data could be various data
>    * structures, policies, and states stored in kernel memory that can
>    * impact the integrity of the system.
>    *
> - * Return: 0 if the buffer has been successfully measured, a negative value
> - * otherwise.
> + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> + * has been written to the passed location but not added to a measurement entry,
> + * a negative value otherwise.
>    */
>   int ima_measure_critical_data(const char *event_label,
>   			      const char *event_name,
>   			      const void *buf, size_t buf_len,
> -			      bool hash)
> +			      bool hash, u8 *digest, size_t digest_len)
>   {
>   	if (!event_name || !event_label || !buf || !buf_len)
>   		return -ENOPARAM;
>   
>   	return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
>   					  event_name, CRITICAL_DATA, 0,
> -					  event_label, hash);
> +					  event_label, hash, digest,
> +					  digest_len);
>   }
>   
>   static int __init init_ima(void)
> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> index e3047ce64f39..b02b061c5fac 100644
> --- a/security/integrity/ima/ima_queue_keys.c
> +++ b/security/integrity/ima/ima_queue_keys.c
> @@ -166,7 +166,7 @@ void ima_process_queued_keys(void)
>   							 entry->keyring_name,
>   							 KEY_CHECK, 0,
>   							 entry->keyring_name,
> -							 false);
> +							 false, NULL, 0);
>   		list_del(&entry->list);
>   		ima_free_key_entry(entry);
>   	}
> diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> index 4db9fa211638..d5d7b3ca9651 100644
> --- a/security/selinux/ima.c
> +++ b/security/selinux/ima.c
> @@ -88,7 +88,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
>   
>   	measure_rc = ima_measure_critical_data("selinux", "selinux-state",
>   					       state_str, strlen(state_str),
> -					       false);
> +					       false, NULL, 0);
>   
>   	kfree(state_str);
>   
> @@ -105,7 +105,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
>   	}
>   
>   	measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
> -					       policy, policy_len, true);
> +					       policy, policy_len, true,
> +					       NULL, 0);
>   
>   	vfree(policy);
>   }
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 2/3] ima: Return int in the functions to measure a buffer
  2021-07-01 16:15   ` Lakshmi Ramasubramanian
@ 2021-07-02 10:51     ` Roberto Sassu
  0 siblings, 0 replies; 9+ messages in thread
From: Roberto Sassu @ 2021-07-02 10:51 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
	linux-security-module, linux-kernel, selinux

> From: Lakshmi Ramasubramanian [mailto:nramas@linux.microsoft.com]
> Sent: Thursday, July 1, 2021 6:16 PM
> On 7/1/2021 5:55 AM, Roberto Sassu wrote:
> > ima_measure_critical_data() and process_buffer_measurement() currently
> > don't return a result. A caller wouldn't be able to know whether those
> > functions were executed successfully.
> >
> > This patch modifies the return type from void to int, and returns 0 if the
> > buffer has been successfully measured, a negative value otherwise.
> >
> > Also, this patch does not modify the behavior of existing callers by
> > processing the returned value. Instead, the value is stored in a variable
> > marked as __maybe_unused.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   include/linux/ima.h                          | 15 +++---
> >   security/integrity/ima/ima.h                 | 10 ++--
> >   security/integrity/ima/ima_appraise.c        |  4 +-
> >   security/integrity/ima/ima_asymmetric_keys.c |  4 +-
> >   security/integrity/ima/ima_init.c            |  6 ++-
> >   security/integrity/ima/ima_main.c            | 48 ++++++++++++--------
> >   security/integrity/ima/ima_queue_keys.c      | 15 +++---
> >   security/selinux/ima.c                       | 10 ++--
> >   8 files changed, 66 insertions(+), 46 deletions(-)
> >
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 81e830d01ced..60492263aa64 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct
> user_namespace *mnt_userns,
> >   extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> >   extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> >   extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > -extern void ima_measure_critical_data(const char *event_label,
> > -				      const char *event_name,
> > -				      const void *buf, size_t buf_len,
> > -				      bool hash);
> > +extern int ima_measure_critical_data(const char *event_label,
> > +				     const char *event_name,
> > +				     const void *buf, size_t buf_len,
> > +				     bool hash);
> >
> >   #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> >   extern void ima_appraise_parse_cmdline(void);
> > @@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode
> *inode, char *buf, size_t buf_size
> >
> >   static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> {}
> >
> > -static inline void ima_measure_critical_data(const char *event_label,
> > +static inline int ima_measure_critical_data(const char *event_label,
> >   					     const char *event_name,
> >   					     const void *buf, size_t buf_len,
> > -					     bool hash) {}
> > +					     bool hash)
> > +{
> > +	return -ENOENT;
> > +}
> >
> >   #endif /* CONFIG_IMA */
> >
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index f0e448ed1f9f..03db221324c3 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -264,11 +264,11 @@ void ima_store_measurement(struct
> integrity_iint_cache *iint, struct file *file,
> >   			   struct evm_ima_xattr_data *xattr_value,
> >   			   int xattr_len, const struct modsig *modsig, int pcr,
> >   			   struct ima_template_desc *template_desc);
> > -void process_buffer_measurement(struct user_namespace *mnt_userns,
> > -				struct inode *inode, const void *buf, int size,
> > -				const char *eventname, enum ima_hooks
> func,
> > -				int pcr, const char *func_data,
> > -				bool buf_hash);
> > +int process_buffer_measurement(struct user_namespace *mnt_userns,
> > +			       struct inode *inode, const void *buf, int size,
> > +			       const char *eventname, enum ima_hooks func,
> > +			       int pcr, const char *func_data,
> > +			       bool buf_hash);
> >   void ima_audit_measurement(struct integrity_iint_cache *iint,
> >   			   const unsigned char *filename);
> >   int ima_alloc_init_template(struct ima_event_data *event_data,
> > diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> > index ef9dcfce45d4..275a2377743f 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -345,6 +345,7 @@ int ima_check_blacklist(struct integrity_iint_cache
> *iint,
> >   	enum hash_algo hash_algo;
> >   	const u8 *digest = NULL;
> >   	u32 digestsize = 0;
> > +	int process_rc __maybe_unused;
> >   	int rc = 0;
> >
> >   	if (!(iint->flags & IMA_CHECK_BLACKLIST))
> > @@ -355,7 +356,8 @@ int ima_check_blacklist(struct integrity_iint_cache
> *iint,
> >
> >   		rc = is_binary_blacklisted(digest, digestsize);
> >   		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> > -			process_buffer_measurement(&init_user_ns, NULL,
> digest, digestsize,
> > +			process_rc =
> process_buffer_measurement(&init_user_ns,
> I think there is no need to make this change now. If and when
> ima_check_blacklist() needs to look at the return value of p_b_m(), this
> change can be made.

Hi Lakshmi

ok. I was worried about possible warnings. If it is not an issue,
I will remove the assignment.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> > +						   NULL, digest, digestsize,
> >   						   "blacklisted-hash", NONE,
> >   						   pcr, NULL, false);
> >   	}
> > diff --git a/security/integrity/ima/ima_asymmetric_keys.c
> b/security/integrity/ima/ima_asymmetric_keys.c
> > index c985418698a4..910367cdd920 100644
> > --- a/security/integrity/ima/ima_asymmetric_keys.c
> > +++ b/security/integrity/ima/ima_asymmetric_keys.c
> > @@ -31,6 +31,7 @@ void ima_post_key_create_or_update(struct key
> *keyring, struct key *key,
> >   				   unsigned long flags, bool create)
> >   {
> >   	bool queued = false;
> > +	int ret __maybe_unused;
> >
> >   	/* Only asymmetric keys are handled by this hook. */
> >   	if (key->type != &key_type_asymmetric)
> > @@ -60,7 +61,8 @@ void ima_post_key_create_or_update(struct key
> *keyring, struct key *key,
> >   	 * if the IMA policy is configured to measure a key linked
> >   	 * to the given keyring.
> >   	 */
> > -	process_buffer_measurement(&init_user_ns, NULL, payload,
> payload_len,
> > +	ret = process_buffer_measurement(&init_user_ns, NULL,
> > +				   payload, payload_len,
> Same comment as in ima_check_blacklist() - this change be made when
> needed.
> 
> >   				   keyring->description, KEY_CHECK, 0,
> >   				   keyring->description, false);
> >   }
> > diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c
> > index 5076a7d9d23e..6790eea88db8 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -118,6 +118,7 @@ void __init ima_load_x509(void)
> >
> >   int __init ima_init(void)
> >   {
> > +	int measure_rc __maybe_unused;
> >   	int rc;
> >
> >   	ima_tpm_chip = tpm_default_chip();
> > @@ -153,8 +154,9 @@ int __init ima_init(void)
> >
> >   	ima_init_key_queue();
> >
> > -	ima_measure_critical_data("kernel_info", "kernel_version",
> > -				  UTS_RELEASE, strlen(UTS_RELEASE), false);
> > +	measure_rc = ima_measure_critical_data("kernel_info",
> "kernel_version",
> > +					       UTS_RELEASE,
> strlen(UTS_RELEASE),
> > +					       false);
> Same comment as in ima_check_blacklist() - this change be made when
> needed.
> 
> >
> >   	return rc;
> >   }
> > diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> > index 8ef1fa357e0c..3386e7436440 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size,
> >   	return 0;
> >   }
> >
> > -/*
> > +/**
> >    * process_buffer_measurement - Measure the buffer or the buffer data
> hash
> >    * @mnt_userns:	user namespace of the mount the inode was found
> from
> >    * @inode: inode associated with the object being measured (NULL for
> KEY_CHECK)
> > @@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size,
> >    * @buf_hash: measure buffer data hash
> >    *
> >    * Based on policy, either the buffer data or buffer data hash is measured
> > + *
> > + * Return: 0 if the buffer has been successfully measured, a negative value
> > + * otherwise.
> >    */
> > -void process_buffer_measurement(struct user_namespace *mnt_userns,
> > -				struct inode *inode, const void *buf, int size,
> > -				const char *eventname, enum ima_hooks
> func,
> > -				int pcr, const char *func_data,
> > -				bool buf_hash)
> > +int process_buffer_measurement(struct user_namespace *mnt_userns,
> > +			       struct inode *inode, const void *buf, int size,
> > +			       const char *eventname, enum ima_hooks func,
> > +			       int pcr, const char *func_data,
> > +			       bool buf_hash)
> >   {
> >   	int ret = 0;
> >   	const char *audit_cause = "ENOMEM";
> > @@ -867,7 +870,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> >   	u32 secid;
> >
> >   	if (!ima_policy_flag)
> > -		return;
> > +		return -ENOENT;
> >
> >   	template = ima_template_desc_buf();
> >   	if (!template) {
> > @@ -889,7 +892,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> >   					secid, 0, func, &pcr, &template,
> >   					func_data);
> >   		if (!(action & IMA_MEASURE))
> > -			return;
> > +			return -ENOENT;
> >   	}
> >
> >   	if (!pcr)
> > @@ -937,7 +940,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> >   					func_measure_str(func),
> >   					audit_cause, ret, 0, ret);
> >
> > -	return;
> > +	return ret;
> >   }
> >
> >   /**
> > @@ -951,6 +954,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> >   void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> >   {
> >   	struct fd f;
> > +	int ret __maybe_unused;
> >
> >   	if (!buf || !size)
> >   		return;
> > @@ -959,9 +963,10 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> >   	if (!f.file)
> >   		return;
> >
> > -	process_buffer_measurement(file_mnt_user_ns(f.file),
> file_inode(f.file),
> > -				   buf, size, "kexec-cmdline", KEXEC_CMDLINE,
> 0,
> > -				   NULL, false);
> > +	ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> > +					 file_inode(f.file), buf, size,
> > +					 "kexec-cmdline", KEXEC_CMDLINE, 0,
> > +					 NULL, false);
> Since the return value of p_b_m() is not used here, this change can be
> made when needed.
> 
> >   	fdput(f);
> >   }
> >
> > @@ -977,18 +982,21 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> >    * and extend the pcr.  Examples of critical data could be various data
> >    * structures, policies, and states stored in kernel memory that can
> >    * impact the integrity of the system.
> > + *
> > + * Return: 0 if the buffer has been successfully measured, a negative value
> > + * otherwise.
> >    */
> > -void ima_measure_critical_data(const char *event_label,
> > -			       const char *event_name,
> > -			       const void *buf, size_t buf_len,
> > -			       bool hash)
> > +int ima_measure_critical_data(const char *event_label,
> > +			      const char *event_name,
> > +			      const void *buf, size_t buf_len,
> > +			      bool hash)
> >   {
> >   	if (!event_name || !event_label || !buf || !buf_len)
> > -		return;
> > +		return -ENOPARAM;
> >
> > -	process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
> event_name,
> > -				   CRITICAL_DATA, 0, event_label,
> > -				   hash);
> > +	return process_buffer_measurement(&init_user_ns, NULL, buf,
> buf_len,
> > +					  event_name, CRITICAL_DATA, 0,
> > +					  event_label, hash);
> >   }
> >
> >   static int __init init_ima(void)
> > diff --git a/security/integrity/ima/ima_queue_keys.c
> b/security/integrity/ima/ima_queue_keys.c
> > index 979ef6c71f3d..e3047ce64f39 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -134,6 +134,7 @@ void ima_process_queued_keys(void)
> >   {
> >   	struct ima_key_entry *entry, *tmp;
> >   	bool process = false;
> > +	int ret __maybe_unused;
> >
> >   	if (ima_process_keys)
> >   		return;
> > @@ -159,13 +160,13 @@ void ima_process_queued_keys(void)
> >
> >   	list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
> >   		if (!timer_expired)
> > -			process_buffer_measurement(&init_user_ns, NULL,
> > -						   entry->payload,
> > -						   entry->payload_len,
> > -						   entry->keyring_name,
> > -						   KEY_CHECK, 0,
> > -						   entry->keyring_name,
> > -						   false);
> > +			ret = process_buffer_measurement(&init_user_ns,
> NULL,
> > +							 entry->payload,
> > +							 entry->payload_len,
> > +							 entry-
> >keyring_name,
> > +							 KEY_CHECK, 0,
> > +							 entry-
> >keyring_name,
> > +							 false);
> Same comment as above.
> 
> >   		list_del(&entry->list);
> >   		ima_free_key_entry(entry);
> >   	}
> > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > index 34d421861bfc..4db9fa211638 100644
> > --- a/security/selinux/ima.c
> > +++ b/security/selinux/ima.c
> > @@ -75,6 +75,7 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> >   	char *state_str = NULL;
> >   	void *policy = NULL;
> >   	size_t policy_len;
> > +	int measure_rc __maybe_unused;
> >   	int rc = 0;
> >
> >   	WARN_ON(!mutex_is_locked(&state->policy_mutex));
> > @@ -85,8 +86,9 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> >   		return;
> >   	}
> >
> > -	ima_measure_critical_data("selinux", "selinux-state",
> > -				  state_str, strlen(state_str), false);
> > +	measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> > +					       state_str, strlen(state_str),
> > +					       false);
> Since the return value of ima_measure_critical_data() is not used here,
> this change can be made when needed.
> 
> >
> >   	kfree(state_str);
> >
> > @@ -102,8 +104,8 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> >   		return;
> >   	}
> >
> > -	ima_measure_critical_data("selinux", "selinux-policy-hash",
> > -				  policy, policy_len, true);
> > +	measure_rc = ima_measure_critical_data("selinux", "selinux-policy-
> hash",
> > +					       policy, policy_len, true);
> Same comment as above.
> 
>   -lakshmi
> 
> >
> >   	vfree(policy);
> >   }
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
  2021-07-01 17:27   ` Lakshmi Ramasubramanian
@ 2021-07-02 10:54     ` Roberto Sassu
  0 siblings, 0 replies; 9+ messages in thread
From: Roberto Sassu @ 2021-07-02 10:54 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, zohar, paul
  Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
	linux-security-module, linux-kernel, selinux

> From: Lakshmi Ramasubramanian [mailto:nramas@linux.microsoft.com]
> Sent: Thursday, July 1, 2021 7:27 PM
> On 7/1/2021 5:55 AM, Roberto Sassu wrote:
> 
> Hi Roberto,
> 
> > This patch adds the 'digest' and 'digest_len' parameters to
> > ima_measure_critical_data() and process_buffer_measurement(), so that
> > callers can get the digest of the passed buffer.
> >
> > These functions calculate the digest even if there is no suitable rule in
> > the IMA policy and, in this case, they simply return 1 before generating a
> > new measurement entry.
> >
> 
> > diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> > index 3386e7436440..b4b1dc25e4fb 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size,
> >    * @pcr: pcr to extend the measurement
> >    * @func_data: func specific data, may be NULL
> >    * @buf_hash: measure buffer data hash
> > + * @digest: buffer digest will be written to
> > + * @digest_len: buffer length
> >    *
> >    * Based on policy, either the buffer data or buffer data hash is measured
> >    *
> > - * Return: 0 if the buffer has been successfully measured, a negative value
> > - * otherwise.
> > + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> > + * has been written to the passed location but not added to a measurement
> entry,
> > + * a negative value otherwise.
> >    */
> >   int process_buffer_measurement(struct user_namespace *mnt_userns,
> >   			       struct inode *inode, const void *buf, int size,
> >   			       const char *eventname, enum ima_hooks func,
> >   			       int pcr, const char *func_data,
> > -			       bool buf_hash)
> > +			       bool buf_hash, u8 *digest, size_t digest_len)
> >   {
> >   	int ret = 0;
> >   	const char *audit_cause = "ENOMEM";
> > @@ -869,7 +872,10 @@ int process_buffer_measurement(struct
> user_namespace *mnt_userns,
> >   	int action = 0;
> >   	u32 secid;
> >
> > -	if (!ima_policy_flag)
> > +	if (digest && digest_len < digest_hash_len)
> > +		return -EINVAL;
> > +
> > +	if (!ima_policy_flag && !digest)
> >   		return -ENOENT;
> 
> Just wanted to check if you have verified this scenario:
> 
> If ima_policy_flag is 0, the in-memory ima policy data is not yet
> initialized. In this case calling ima_get_action() will cause kernel
> panic (NULL exception).
> 
> Please verify the above issue doesn't exist if the caller passes
> non-NULL digest and ima_policy_flag is 0 (ima policy is not initialized).

Yes, it is fixed with commit 067a436b1b0aa.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> thanks,
>   -lakshmi
> 
> >
> >   	template = ima_template_desc_buf();
> > @@ -891,7 +897,7 @@ int process_buffer_measurement(struct
> user_namespace *mnt_userns,
> >   		action = ima_get_action(mnt_userns, inode, current_cred(),
> >   					secid, 0, func, &pcr, &template,
> >   					func_data);
> > -		if (!(action & IMA_MEASURE))
> > +		if (!(action & IMA_MEASURE) && !digest)
> >   			return -ENOENT;
> >   	}
> >
> > @@ -922,6 +928,12 @@ int process_buffer_measurement(struct
> user_namespace *mnt_userns,
> >   		event_data.buf_len = digest_hash_len;
> >   	}
> >
> > +	if (digest)
> > +		memcpy(digest, iint.ima_hash->digest, digest_hash_len);
> > +
> > +	if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
> > +		return 1;
> > +
> >   	ret = ima_alloc_init_template(&event_data, &entry, template);
> >   	if (ret < 0) {
> >   		audit_cause = "alloc_entry";
> > @@ -966,7 +978,7 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> >   	ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> >   					 file_inode(f.file), buf, size,
> >   					 "kexec-cmdline", KEXEC_CMDLINE, 0,
> > -					 NULL, false);
> > +					 NULL, false, NULL, 0);
> >   	fdput(f);
> >   }
> >
> > @@ -977,26 +989,30 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> >    * @buf: pointer to buffer data
> >    * @buf_len: length of buffer data (in bytes)
> >    * @hash: measure buffer data hash
> > + * @digest: buffer digest will be written to
> > + * @digest_len: buffer length
> >    *
> >    * Measure data critical to the integrity of the kernel into the IMA log
> >    * and extend the pcr.  Examples of critical data could be various data
> >    * structures, policies, and states stored in kernel memory that can
> >    * impact the integrity of the system.
> >    *
> > - * Return: 0 if the buffer has been successfully measured, a negative value
> > - * otherwise.
> > + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> > + * has been written to the passed location but not added to a measurement
> entry,
> > + * a negative value otherwise.
> >    */
> >   int ima_measure_critical_data(const char *event_label,
> >   			      const char *event_name,
> >   			      const void *buf, size_t buf_len,
> > -			      bool hash)
> > +			      bool hash, u8 *digest, size_t digest_len)
> >   {
> >   	if (!event_name || !event_label || !buf || !buf_len)
> >   		return -ENOPARAM;
> >
> >   	return process_buffer_measurement(&init_user_ns, NULL, buf,
> buf_len,
> >   					  event_name, CRITICAL_DATA, 0,
> > -					  event_label, hash);
> > +					  event_label, hash, digest,
> > +					  digest_len);
> >   }
> >
> >   static int __init init_ima(void)
> > diff --git a/security/integrity/ima/ima_queue_keys.c
> b/security/integrity/ima/ima_queue_keys.c
> > index e3047ce64f39..b02b061c5fac 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -166,7 +166,7 @@ void ima_process_queued_keys(void)
> >   							 entry-
> >keyring_name,
> >   							 KEY_CHECK, 0,
> >   							 entry-
> >keyring_name,
> > -							 false);
> > +							 false, NULL, 0);
> >   		list_del(&entry->list);
> >   		ima_free_key_entry(entry);
> >   	}
> > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > index 4db9fa211638..d5d7b3ca9651 100644
> > --- a/security/selinux/ima.c
> > +++ b/security/selinux/ima.c
> > @@ -88,7 +88,7 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> >
> >   	measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> >   					       state_str, strlen(state_str),
> > -					       false);
> > +					       false, NULL, 0);
> >
> >   	kfree(state_str);
> >
> > @@ -105,7 +105,8 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> >   	}
> >
> >   	measure_rc = ima_measure_critical_data("selinux", "selinux-policy-
> hash",
> > -					       policy, policy_len, true);
> > +					       policy, policy_len, true,
> > +					       NULL, 0);
> >
> >   	vfree(policy);
> >   }
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-02 10:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-01 12:55 [PATCH v2 0/3] ima: Provide more info about buffer measurement Roberto Sassu
2021-07-01 12:55 ` [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo() Roberto Sassu
2021-07-01 16:01   ` Lakshmi Ramasubramanian
2021-07-01 12:55 ` [PATCH v2 2/3] ima: Return int in the functions to measure a buffer Roberto Sassu
2021-07-01 16:15   ` Lakshmi Ramasubramanian
2021-07-02 10:51     ` Roberto Sassu
2021-07-01 12:55 ` [PATCH v2 3/3] ima: Add digest and digest_len params to " Roberto Sassu
2021-07-01 17:27   ` Lakshmi Ramasubramanian
2021-07-02 10:54     ` Roberto Sassu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).