Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima
@ 2020-02-11  2:47 Tushar Sugandhi
  2020-02-11  2:47 ` [PATCH v2 2/3] IMA: Add log statements for failure conditions Tushar Sugandhi
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Tushar Sugandhi @ 2020-02-11  2:47 UTC (permalink / raw)
  To: zohar, joe, skhan, linux-integrity; +Cc: sashal, nramas, linux-kernel

Log statements from ima_mok.c, ima_asymmetric_keys.c, and
ima_queue_keys.c are prefixed with the respective file names
and not with the string "ima". 

This change fixes the log statement prefix to be consistent with the rest
of the IMA files.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 064a256f8725..67dabca670e2 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -11,6 +11,6 @@ ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
 ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
-obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
-obj-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
-obj-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
+ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
+ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
+ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
-- 
2.17.1


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

* [PATCH v2 2/3] IMA: Add log statements for failure conditions.
  2020-02-11  2:47 [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Tushar Sugandhi
@ 2020-02-11  2:47 ` Tushar Sugandhi
  2020-02-11  3:23   ` Joe Perches
  2020-02-11  2:47 ` [PATCH v2 3/3] IMA: Add module name and base name prefix to log Tushar Sugandhi
  2020-02-11  3:09 ` [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Joe Perches
  2 siblings, 1 reply; 9+ messages in thread
From: Tushar Sugandhi @ 2020-02-11  2:47 UTC (permalink / raw)
  To: zohar, joe, skhan, linux-integrity; +Cc: sashal, nramas, linux-kernel

process_buffer_measurement() and ima_alloc_key_entry()
functions do not have log messages for failure conditions.

This change adds log statements in the above functions. 

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_main.c       | 3 +++
 security/integrity/ima/ima_queue_keys.c | 1 +
 2 files changed, 4 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9fe949c6a530..ee01ee34eec8 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -757,6 +757,9 @@ void process_buffer_measurement(const void *buf, int size,
 		ima_free_template_entry(entry);
 
 out:
+	if (ret < 0)
+		pr_err("Process buffer measurement failed, result: %d\n", ret);
+
 	return;
 }
 
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index c87c72299191..6a9ee52649c4 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -90,6 +90,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
 
 out:
 	if (rc) {
+		pr_err("Key entry allocation failed, result: %d\n", rc);
 		ima_free_key_entry(entry);
 		entry = NULL;
 	}
-- 
2.17.1


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

* [PATCH v2 3/3] IMA: Add module name and base name prefix to log
  2020-02-11  2:47 [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Tushar Sugandhi
  2020-02-11  2:47 ` [PATCH v2 2/3] IMA: Add log statements for failure conditions Tushar Sugandhi
@ 2020-02-11  2:47 ` Tushar Sugandhi
  2020-02-11  3:09 ` [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Joe Perches
  2 siblings, 0 replies; 9+ messages in thread
From: Tushar Sugandhi @ 2020-02-11  2:47 UTC (permalink / raw)
  To: zohar, joe, skhan, linux-integrity; +Cc: sashal, nramas, linux-kernel

The #define for formatting log messages, pr_fmt, is duplicated in the
files under security/integrity.

This change moves the definition to security/integrity/integrity.h and
removes the duplicate definitions in the other files under
security/integrity. Also, it adds KBUILD_MODNAME and KBUILD_BASENAME prefix
to the log messages.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Suggested-by: Joe Perches <joe@perches.com>
Suggested-by: Shuah Khan <skhan@linuxfoundation.org>
---
 security/integrity/digsig.c                  | 2 --
 security/integrity/digsig_asymmetric.c       | 2 --
 security/integrity/evm/evm_crypto.c          | 2 --
 security/integrity/evm/evm_main.c            | 2 --
 security/integrity/evm/evm_secfs.c           | 2 --
 security/integrity/ima/ima_asymmetric_keys.c | 2 --
 security/integrity/ima/ima_crypto.c          | 2 --
 security/integrity/ima/ima_fs.c              | 2 --
 security/integrity/ima/ima_init.c            | 2 --
 security/integrity/ima/ima_kexec.c           | 1 -
 security/integrity/ima/ima_main.c            | 2 --
 security/integrity/ima/ima_policy.c          | 2 --
 security/integrity/ima/ima_queue.c           | 2 --
 security/integrity/ima/ima_queue_keys.c      | 2 --
 security/integrity/ima/ima_template.c        | 2 --
 security/integrity/ima/ima_template_lib.c    | 2 --
 security/integrity/integrity.h               | 6 ++++++
 17 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index ea1aae3d07b3..e9cbadade74b 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -6,8 +6,6 @@
  * Dmitry Kasatkin <dmitry.kasatkin@intel.com>
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/err.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index 55aec161d0e1..4e0d6778277e 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -6,8 +6,6 @@
  * Dmitry Kasatkin <dmitry.kasatkin@intel.com>
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/err.h>
 #include <linux/ratelimit.h>
 #include <linux/key-type.h>
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index d485f6fc908e..35682852ddea 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -10,8 +10,6 @@
  *	 Using root's kernel master key (kmk), calculate the HMAC
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/export.h>
 #include <linux/crypto.h>
 #include <linux/xattr.h>
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index f9a81b187fae..d361d7fdafc4 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -11,8 +11,6 @@
  *	evm_inode_removexattr, and evm_verifyxattr
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/init.h>
 #include <linux/crypto.h>
 #include <linux/audit.h>
diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index c11c1f7b3ddd..39ad1038d45d 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -10,8 +10,6 @@
  *	- Get the key and enable EVM
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/audit.h>
 #include <linux/uaccess.h>
 #include <linux/init.h>
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 7678f0e3e84d..aaae80c4e376 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -9,8 +9,6 @@
  *       create or update.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <keys/asymmetric-type.h>
 #include "ima.h"
 
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7967a6904851..423c84f95a14 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -10,8 +10,6 @@
  *	Calculates md5/sha1 file hash, template hash, boot-aggreate hash
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
 #include <linux/ratelimit.h>
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 2000e8df0301..a71e822a6e92 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -12,8 +12,6 @@
  *	current measurement list and IMA statistics
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/fcntl.h>
 #include <linux/slab.h>
 #include <linux/init.h>
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 195cb4079b2b..567468188a61 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -11,8 +11,6 @@
  *             initialization and cleanup functions
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/init.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 9e94eca48b89..121de3e04af2 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -6,7 +6,6 @@
  * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
  * Mimi Zohar <zohar@linux.vnet.ibm.com>
  */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/seq_file.h>
 #include <linux/vmalloc.h>
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index ee01ee34eec8..e78d0aa665f3 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -15,8 +15,6 @@
  *	and ima_file_check.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/module.h>
 #include <linux/file.h>
 #include <linux/binfmts.h>
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 453427048999..c334e0dc6083 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -7,8 +7,6 @@
  *	- initialize default measure policy rules
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/fs.h>
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 1ce8b1701566..8753212ddb18 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -15,8 +15,6 @@
  *       ever removed or changed during the boot-cycle.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/rculist.h>
 #include <linux/slab.h>
 #include "ima.h"
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 6a9ee52649c4..ffd78f09c5b6 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -8,8 +8,6 @@
  *       Enables deferred processing of keys
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/workqueue.h>
 #include <keys/asymmetric-type.h>
 #include "ima.h"
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 6aa6408603e3..062d9ad49afb 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -9,8 +9,6 @@
  *      Helpers to manage template descriptors.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/rculist.h>
 #include "ima.h"
 #include "ima_template_lib.h"
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 32ae05d88257..9cd1e50f3ccc 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -9,8 +9,6 @@
  *      Library of supported template fields.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include "ima_template_lib.h"
 
 static bool ima_template_hash_algo_allowed(u8 algo)
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 73fc286834d7..b1bb4d2263be 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -6,6 +6,12 @@
  * Mimi Zohar <zohar@us.ibm.com>
  */
 
+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt
+
 #include <linux/types.h>
 #include <linux/integrity.h>
 #include <crypto/sha.h>
-- 
2.17.1


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

* Re: [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima
  2020-02-11  2:47 [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Tushar Sugandhi
  2020-02-11  2:47 ` [PATCH v2 2/3] IMA: Add log statements for failure conditions Tushar Sugandhi
  2020-02-11  2:47 ` [PATCH v2 3/3] IMA: Add module name and base name prefix to log Tushar Sugandhi
@ 2020-02-11  3:09 ` Joe Perches
  2020-02-11 17:36   ` Lakshmi Ramasubramanian
  2 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2020-02-11  3:09 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, skhan, linux-integrity
  Cc: sashal, nramas, linux-kernel

On Mon, 2020-02-10 at 18:47 -0800, Tushar Sugandhi wrote:
> Log statements from ima_mok.c, ima_asymmetric_keys.c, and
> ima_queue_keys.c are prefixed with the respective file names
> and not with the string "ima". 

Series seems sensible, thanks Tushar.

Next time you might choose to use

	git format-patch --cover-letter

and write in the 0/n cover letter what the point
of the patch series is.

cheers and welcome... Joe


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

* Re: [PATCH v2 2/3] IMA: Add log statements for failure conditions.
  2020-02-11  2:47 ` [PATCH v2 2/3] IMA: Add log statements for failure conditions Tushar Sugandhi
@ 2020-02-11  3:23   ` Joe Perches
  2020-02-11 19:14     ` Tushar Sugandhi
  0 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2020-02-11  3:23 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, skhan, linux-integrity
  Cc: sashal, nramas, linux-kernel

On Mon, 2020-02-10 at 18:47 -0800, Tushar Sugandhi wrote:
> process_buffer_measurement() and ima_alloc_key_entry()
> functions do not have log messages for failure conditions.

trivia:

> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
[]
> @@ -757,6 +757,9 @@ void process_buffer_measurement(const void *buf, int size,
>  		ima_free_template_entry(entry);
>  
>  out:
> +	if (ret < 0)
> +		pr_err("Process buffer measurement failed, result: %d\n", ret);

perhaps use %s, __func__

		pr_err("%s: failed, result: %d\n", __func__, ret);
 
> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
[]
> @@ -90,6 +90,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
>  
>  out:
>  	if (rc) {
> +		pr_err("Key entry allocation failed, result: %d\n", rc);
>  		ima_free_key_entry(entry);
>  		entry = NULL;
>  	}

Likely the pr_err is unnecessary here as kmalloc, kstrdup
and kmemdup all emit a dump_stack() on allocation failure.

Perhaps instead:

o Remove unnecessary indentation in ima_free_key_entry by
  returning early on NULL argument
o Remove unnecessary rc, tests and label in ima_alloc_key_entry
---
 security/integrity/ima/ima_queue_keys.c | 37 +++++++++++++--------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index c87c722..ba449f 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -58,42 +58,35 @@ void ima_init_key_queue(void)
 
 static void ima_free_key_entry(struct ima_key_entry *entry)
 {
-	if (entry) {
-		kfree(entry->payload);
-		kfree(entry->keyring_name);
-		kfree(entry);
-	}
+	if (!entry)
+		return;
+
+	kfree(entry->payload);
+	kfree(entry->keyring_name);
+	kfree(entry);
 }
 
 static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
 						 const void *payload,
 						 size_t payload_len)
 {
-	int rc = 0;
 	struct ima_key_entry *entry;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
-	if (entry) {
-		entry->payload = kmemdup(payload, payload_len, GFP_KERNEL);
-		entry->keyring_name = kstrdup(keyring->description,
-					      GFP_KERNEL);
-		entry->payload_len = payload_len;
-	}
-
-	if ((entry == NULL) || (entry->payload == NULL) ||
-	    (entry->keyring_name == NULL)) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!entry)
+		return NULL;
 
-	INIT_LIST_HEAD(&entry->list);
+	entry->payload = kmemdup(payload, payload_len, GFP_KERNEL);
+	entry->payload_len = payload_len;
+	entry->keyring_name = kstrdup(keyring->description, GFP_KERNEL);
 
-out:
-	if (rc) {
+	if (!entry->payload || !entry->keyring_name) {
 		ima_free_key_entry(entry);
-		entry = NULL;
+		return NULL;
 	}
 
+	INIT_LIST_HEAD(&entry->list);
+
 	return entry;
 }
 



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

* Re: [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima
  2020-02-11  3:09 ` [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Joe Perches
@ 2020-02-11 17:36   ` Lakshmi Ramasubramanian
  2020-02-11 17:55     ` Tushar Sugandhi
  0 siblings, 1 reply; 9+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-02-11 17:36 UTC (permalink / raw)
  To: Joe Perches, Tushar Sugandhi, zohar, skhan, linux-integrity
  Cc: sashal, linux-kernel

On 2/10/20 7:09 PM, Joe Perches wrote:

Hi Joe,

> Series seems sensible, thanks Tushar.
> 
> Next time you might choose to use
> 
> 	git format-patch --cover-letter
> 
> and write in the 0/n cover letter what the point
> of the patch series is.
> 

I was the one who suggested that we may not need a cover letter and 
instead provide the details in the patch description. We can include the 
cover letter in the next update.

thanks,
  -lakshmi



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

* Re: [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima
  2020-02-11 17:36   ` Lakshmi Ramasubramanian
@ 2020-02-11 17:55     ` Tushar Sugandhi
  0 siblings, 0 replies; 9+ messages in thread
From: Tushar Sugandhi @ 2020-02-11 17:55 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, Joe Perches, zohar, skhan, linux-integrity
  Cc: sashal, linux-kernel

Hi Joe,

On 2020-02-11 9:36 a.m., Lakshmi Ramasubramanian wrote:
> On 2/10/20 7:09 PM, Joe Perches wrote:
> 
> Hi Joe,
> 
>> Series seems sensible, thanks Tushar.
>>
>> Next time you might choose to use
>>
>>     git format-patch --cover-letter
>>
>> and write in the 0/n cover letter what the point
>> of the patch series is.
>>
> 
> I was the one who suggested that we may not need a cover letter and 
> instead provide the details in the patch description. We can include the 
> cover letter in the next update.
> 
> thanks,
>   -lakshmi
> 
I will include the cover letter in the next update.

Thanks,
Tushar

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

* Re: [PATCH v2 2/3] IMA: Add log statements for failure conditions.
  2020-02-11  3:23   ` Joe Perches
@ 2020-02-11 19:14     ` Tushar Sugandhi
  2020-02-11 20:09       ` Joe Perches
  0 siblings, 1 reply; 9+ messages in thread
From: Tushar Sugandhi @ 2020-02-11 19:14 UTC (permalink / raw)
  To: Joe Perches, zohar, skhan, linux-integrity; +Cc: sashal, nramas, linux-kernel

Hi Joe,

On 2020-02-10 7:23 p.m., Joe Perches wrote:
> On Mon, 2020-02-10 at 18:47 -0800, Tushar Sugandhi wrote:
>> process_buffer_measurement() and ima_alloc_key_entry()
>> functions do not have log messages for failure conditions.
> 
> trivia:
> 
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> []
>> @@ -757,6 +757,9 @@ void process_buffer_measurement(const void *buf, int size,
>>   		ima_free_template_entry(entry);
>>   
>>   out:
>> +	if (ret < 0)
>> +		pr_err("Process buffer measurement failed, result: %d\n", ret);
> 
> perhaps use %s, __func__
> 
> 		pr_err("%s: failed, result: %d\n", __func__, ret);
>   
Sounds good. Will make this change in the next update.
>> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> []
>> @@ -90,6 +90,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
>>   
>>   out:
>>   	if (rc) {
>> +		pr_err("Key entry allocation failed, result: %d\n", rc);
>>   		ima_free_key_entry(entry);
>>   		entry = NULL;
>>   	}
> 
> Likely the pr_err is unnecessary here as kmalloc, kstrdup
> and kmemdup all emit a dump_stack() on allocation failure.
Thanks for pointing out kmalloc, kstrdup, and kmemdup emit a 
dump_stack(). But keeping the above pr_err() will help associate the 
failure with IMA.
For instance - "dmesg | grep ima:" will include this error.
Perhaps I should add __func__ here as well.
And since we are redefining the pr_fmt to prefix module and base names, 
it will help further to pinpoint where exactly the failure is coming from.

> 
> Perhaps instead:
> 
> o Remove unnecessary indentation in ima_free_key_entry by
>    returning early on NULL argument
> o Remove unnecessary rc, tests and label in ima_alloc_key_entry
> ---
>   security/integrity/ima/ima_queue_keys.c | 37 +++++++++++++--------------------
>   1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> index c87c722..ba449f 100644
> --- a/security/integrity/ima/ima_queue_keys.c
> +++ b/security/integrity/ima/ima_queue_keys.c
> @@ -58,42 +58,35 @@ void ima_init_key_queue(void)
>   
>   static void ima_free_key_entry(struct ima_key_entry *entry)
>   {
> -	if (entry) {
> -		kfree(entry->payload);
> -		kfree(entry->keyring_name);
> -		kfree(entry);
> -	}
> +	if (!entry)
> +		return;
> +
> +	kfree(entry->payload);
> +	kfree(entry->keyring_name);
> +	kfree(entry);
>   }
>   
Thanks for the feedback. Appreciate it. I would like to make this fix.
But I am not sure if this patchset, which mainly focuses on improving 
logging in IMA, is the right patchset for this.
>   static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
>   						 const void *payload,
>   						 size_t payload_len)
>   {
> -	int rc = 0;
>   	struct ima_key_entry *entry;
>   
>   	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> -	if (entry) {
> -		entry->payload = kmemdup(payload, payload_len, GFP_KERNEL);
> -		entry->keyring_name = kstrdup(keyring->description,
> -					      GFP_KERNEL);
> -		entry->payload_len = payload_len;
> -	}
> -
> -	if ((entry == NULL) || (entry->payload == NULL) ||
> -	    (entry->keyring_name == NULL)) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> +	if (!entry)
> +		return NULL;
>   
> -	INIT_LIST_HEAD(&entry->list);
> +	entry->payload = kmemdup(payload, payload_len, GFP_KERNEL);
> +	entry->payload_len = payload_len;
> +	entry->keyring_name = kstrdup(keyring->description, GFP_KERNEL);
>   
> -out:
> -	if (rc) {
> +	if (!entry->payload || !entry->keyring_name) {
>   		ima_free_key_entry(entry);
> -		entry = NULL;
> +		return NULL;
>   	}
>   
> +	INIT_LIST_HEAD(&entry->list);
> +
>   	return entry;
>   }
>   
> 
Thanks again. This recommended change certainly makes the code more 
readable. But again, I am not sure if this patchset is the right one for 
this proposed change.
Perhaps I can create another patchset for the above two recommended 
changes, and only focus on improving logging in this patchset?

Thanks,
Tushar

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

* Re: [PATCH v2 2/3] IMA: Add log statements for failure conditions.
  2020-02-11 19:14     ` Tushar Sugandhi
@ 2020-02-11 20:09       ` Joe Perches
  0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2020-02-11 20:09 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, skhan, linux-integrity
  Cc: sashal, nramas, linux-kernel

On Tue, 2020-02-11 at 11:14 -0800, Tushar Sugandhi wrote:
> Hi Joe,

Rehi Tushar.

> On 2020-02-10 7:23 p.m., Joe Perches wrote:
> > On Mon, 2020-02-10 at 18:47 -0800, Tushar Sugandhi wrote:
> > > process_buffer_measurement() and ima_alloc_key_entry()
> > > functions do not have log messages for failure conditions.
[]
> > > diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> > []
> > > @@ -90,6 +90,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
> > >   
> > >   out:
> > >   	if (rc) {
> > > +		pr_err("Key entry allocation failed, result: %d\n", rc);
> > >   		ima_free_key_entry(entry);
> > >   		entry = NULL;
> > >   	}
> > 
> > Likely the pr_err is unnecessary here as kmalloc, kstrdup
> > and kmemdup all emit a dump_stack() on allocation failure.
> Thanks for pointing out kmalloc, kstrdup, and kmemdup emit a 
> dump_stack(). But keeping the above pr_err() will help associate the 
> failure with IMA.
> For instance - "dmesg | grep ima:" will include this error.
> Perhaps I should add __func__ here as well.
> And since we are redefining the pr_fmt to prefix module and base names, 
> it will help further to pinpoint where exactly the failure is coming from.

The dump_stack is preferred over a single printk message
and the association isn't particularly useful.

> Thanks again. This recommended change certainly makes the code more 
> readable. But again, I am not sure if this patchset is the right one for 
> this proposed change.
> Perhaps I can create another patchset for the above two recommended 
> changes, and only focus on improving logging in this patchset?

Your choice.



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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11  2:47 [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Tushar Sugandhi
2020-02-11  2:47 ` [PATCH v2 2/3] IMA: Add log statements for failure conditions Tushar Sugandhi
2020-02-11  3:23   ` Joe Perches
2020-02-11 19:14     ` Tushar Sugandhi
2020-02-11 20:09       ` Joe Perches
2020-02-11  2:47 ` [PATCH v2 3/3] IMA: Add module name and base name prefix to log Tushar Sugandhi
2020-02-11  3:09 ` [PATCH v2 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima Joe Perches
2020-02-11 17:36   ` Lakshmi Ramasubramanian
2020-02-11 17:55     ` Tushar Sugandhi

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git