Skip to contentSkip to conversation

IMA: improve log messages in IMA

PATCH Superseded Tushar Sugandhi 5 participants 18 messages Feb 11, 2020 – Feb 13, 2020

Subsystems (4)
Security (LSM) Keys-trusted Integrity measurement architecture (IMA) Extended Verification Module (EVM)

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Tushar Sugandhi

author
#0 lore raw

v3 cover IMA: improve log messages in IMA

Some files under IMA prefix the log statement with the respective file names and not with the string "ima". This is not consistent with the rest of the IMA files.

The function process_buffer_measurement() does not have log messages for failure conditions.

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

This patchset addresses the above issues.

Tushar Sugandhi (3):
add log prefix to ima_mok.c, ima_asymmetric_keys.c, ima_queue_keys.c add log message to process_buffer_measurement failure conditions add module name and base name prefix to log statements

 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/Makefile              | 6 +++---
 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            | 5 +++--
 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 ++++++
 18 files changed, 12 insertions(+), 34 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v3 1/3 IMA: Update KBUILD_MODNAME for IMA files to ima

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 <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
---
 security/integrity/ima/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
security/integrity/ima/Makefile +3 -3
index 064a256f8725..67dabca670e2 100644
@@ -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
Signature · 1 line
2.17.1
maintainer
#2 lore raw

Re: [PATCH v3 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima

Quoted 3 lines · Tushar Sugandhi

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".

Before listing the specific filenames, the patch description should provide a generic explanation of the problem.  For example, the kernel Makefile "obj-$CONFIG_XXXX" specifies object files which may be built as loadable kernel modules[1].

Mimi

[1] Refer to Documentation/kbuild/makefiles.rst

Quoted 24 lines

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

Signed-off-by Tushar Sugandhi <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
---
 security/integrity/ima/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
security/integrity/ima/Makefile +3 -3
index 064a256f8725..67dabca670e2 100644
@@ -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
author
#3 lore raw

Re: [PATCH v3 1/3] IMA: Update KBUILD_MODNAME for IMA files to ima

Quoted 10 lines · Mimi Zohar
Quoted 3 lines · Tushar Sugandhi

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".

Before listing the specific filenames, the patch description should provide a generic explanation of the problem.  For example, the kernel Makefile "obj-$CONFIG_XXXX" specifies object files which may be built as loadable kernel modules[1].

Thanks Mimi. I will update the patch description in the next iteration.

Quoted 28 lines

Mimi

[1] Refer to Documentation/kbuild/makefiles.rst

Quoted 24 lines

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

Signed-off-by Tushar Sugandhi <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
---
  security/integrity/ima/Makefile | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
security/integrity/ima/Makefile +3 -3
index 064a256f8725..67dabca670e2 100644
@@ -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
author
#4 lore raw

PATCH v3 2/3 IMA: Add log statements for failure conditions.

In reply to Tushar Sugandhi

process_buffer_measurement() does not have log messages for failure conditions.

This change adds a log statement in the above function.

Signed-off-by Tushar Sugandhi <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
---
 security/integrity/ima/ima_main.c | 3 +++
 1 file changed, 3 insertions(+)
security/integrity/ima/ima_main.c +3 -0
index 9fe949c6a530..6e1576d9eb48 100644
@@ -757,6 +757,9 @@ void process_buffer_measurement(const void *buf, int size,
 		ima_free_template_entry(entry);
 
 out:
+	if (ret < 0)
+		pr_err("%s: failed, result: %d\n", __func__, ret);
+
 	return;
 }
Signature · 1 line
2.17.1
maintainer
#5 lore raw

Re: [PATCH v3 2/3] IMA: Add log statements for failure conditions.

Hi Tushar,

Please remove the period at the end of the  Subject line.

Quoted 4 lines · Tushar Sugandhi

process_buffer_measurement() does not have log messages for failure conditions.

This change adds a log statement in the above function.

I agree some form of notification needs to be added.  The question is whether the failure should be audited or a kernel message emitted. IMA emits audit messages (integrity_audit_msg) for a number of reasons - on failure to calculate a file hash, invalid policy rules, failure to communicate with the TPM, signature verification errors, etc.

Quoted 13 lines
Signed-off-by Tushar Sugandhi <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
---
 security/integrity/ima/ima_main.c | 3 +++
 1 file changed, 3 insertions(+)
security/integrity/ima/ima_main.c +0 -0
index 9fe949c6a530..6e1576d9eb48 100644

security/integrity/ima/ima_main.c void process_buffer_measurement(const void *buf, int size, · line 765

Quoted 9 lines
security/integrity/ima/ima_main.c +3 -0
@@ -757,6 +757,9 @@ void process_buffer_measurement(const void *buf, int size,
 		ima_free_template_entry(entry);
 
 out:
+	if (ret < 0)
+		pr_err("%s: failed, result: %d\n", __func__, ret);
+
 	return;
 }

With 3/3 "IMA: Add module name and base name prefix to log", the resulting message will be "KBUILD_MODNAME: KBUILD_BASENAME: func:". Isn't that a bit much?

Mimi

author
#6 lore raw

Re: [PATCH v3 2/3] IMA: Add log statements for failure conditions.

Quoted 3 lines · Mimi Zohar

Hi Tushar,

Please remove the period at the end of the  Subject line.

Thanks. I will fix it in the next iteration.

Quoted 13 lines · Tushar Sugandhi
Quoted 4 lines

process_buffer_measurement() does not have log messages for failure conditions.

This change adds a log statement in the above function.

I agree some form of notification needs to be added.  The question is whether the failure should be audited or a kernel message emitted.
 IMA emits audit messages (integrity_audit_msg) for a number of
reasons - on failure to calculate a file hash, invalid policy rules, failure to communicate with the TPM, signature verification errors, etc.

I believe both IMA audit messages and kernel message should be emitted - for better discoverability and diagnosability.

Quoted 16 lines
Quoted 16 lines
Signed-off-by Tushar Sugandhi <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
---
  security/integrity/ima/ima_main.c | 3 +++
  1 file changed, 3 insertions(+)
security/integrity/ima/ima_main.c +0 -0
index 9fe949c6a530..6e1576d9eb48 100644
@@ -757,6 +757,9 @@ void process_buffer_measurement(const void *buf, int size,
  		ima_free_template_entry(entry);
Quoted 12 lines

security/integrity/ima/ima_main.c void process_buffer_measurement(const void *buf, int size, · line 765

Quoted 7 lines
security/integrity/ima/ima_main.c +3 -0
  out:
+	if (ret < 0)
+		pr_err("%s: failed, result: %d\n", __func__, ret);
+
  	return;
  }

With 3/3 "IMA: Add module name and base name prefix to log", the resulting message will be "KBUILD_MODNAME: KBUILD_BASENAME: func:".
 Isn't that a bit much?

For this specific message, it will look like below. "ima: ima_main: process_buffer_measurement: failed, result: %d"

In general, adding KBUILD_BASENAME seems helpful to pinpoint the location of the issue.

Quoted 2 lines

Mimi

maintainer
#7 lore raw

Re: [PATCH v3 2/3] IMA: Add log statements for failure conditions.

Quoted 13 lines · Tushar Sugandhi
Quoted 3 lines · Mimi Zohar

Hi Tushar,

Please remove the period at the end of the  Subject line.

Thanks. I will fix it in the next iteration.

Quoted 7 lines · Tushar Sugandhi
Quoted 4 lines

process_buffer_measurement() does not have log messages for failure conditions.

This change adds a log statement in the above function.

Quoted 8 lines
Quoted 6 lines

I agree some form of notification needs to be added.  The question is whether the failure should be audited or a kernel message emitted.
 IMA emits audit messages (integrity_audit_msg) for a number of
reasons - on failure to calculate a file hash, invalid policy rules, failure to communicate with the TPM, signature verification errors, etc.

I believe both IMA audit messages and kernel message should be emitted - for better discoverability and diagnosability.

Like file measurement failures, failure to measure a key or the boot command line should be audited as well.  For debugging purposes, you could make this message pr_devel.

Mimi

author
#8 lore raw

Re: [PATCH v3 2/3] IMA: Add log statements for failure conditions.

Quoted 20 lines · Mimi Zohar
Quoted 19 lines · Tushar Sugandhi
Quoted 3 lines · Mimi Zohar

Hi Tushar,

Please remove the period at the end of the  Subject line.

Thanks. I will fix it in the next iteration.

Quoted 13 lines · Tushar Sugandhi
Quoted 4 lines

process_buffer_measurement() does not have log messages for failure conditions.

This change adds a log statement in the above function.

I agree some form of notification needs to be added.  The question is whether the failure should be audited or a kernel message emitted.
 IMA emits audit messages (integrity_audit_msg) for a number of
reasons - on failure to calculate a file hash, invalid policy rules, failure to communicate with the TPM, signature verification errors, etc.

Quoted 6 lines
Quoted 2 lines

I believe both IMA audit messages and kernel message should be emitted - for better discoverability and diagnosability.

Like file measurement failures, failure to measure a key or the boot command line should be audited as well.  For debugging purposes, you could make this message pr_devel.

Ok. I will change this to pr_devel in next iteration.

Quoted 3 lines

Mimi

author
#9 lore raw

PATCH v3 3/3 IMA: Add module name and base name prefix to log.

In reply to Tushar Sugandhi

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 <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
Suggested-by Shuah Khan <[email protected]>
---
 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(-)
security/integrity/digsig.c +0 -2
index ea1aae3d07b3..e9cbadade74b 100644
@@ -6,8 +6,6 @@
  * Dmitry Kasatkin <[email protected]>
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/err.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
security/integrity/digsig_asymmetric.c +0 -2
index 55aec161d0e1..4e0d6778277e 100644
@@ -6,8 +6,6 @@
  * Dmitry Kasatkin <[email protected]>
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/err.h>
 #include <linux/ratelimit.h>
 #include <linux/key-type.h>
security/integrity/evm/evm_crypto.c +0 -2
index d485f6fc908e..35682852ddea 100644
@@ -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>
security/integrity/evm/evm_main.c +0 -2
index f9a81b187fae..d361d7fdafc4 100644
@@ -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>
security/integrity/evm/evm_secfs.c +0 -2
index c11c1f7b3ddd..39ad1038d45d 100644
@@ -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>
security/integrity/ima/ima_asymmetric_keys.c +0 -2
index 7678f0e3e84d..aaae80c4e376 100644
@@ -9,8 +9,6 @@
  *       create or update.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <keys/asymmetric-type.h>
 #include "ima.h"
security/integrity/ima/ima_crypto.c +0 -2
index 7967a6904851..423c84f95a14 100644
@@ -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>
security/integrity/ima/ima_fs.c +0 -2
index 2000e8df0301..a71e822a6e92 100644
@@ -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>
security/integrity/ima/ima_init.c +0 -2
index 195cb4079b2b..567468188a61 100644
@@ -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>
security/integrity/ima/ima_kexec.c +0 -1
index 9e94eca48b89..121de3e04af2 100644
@@ -6,7 +6,6 @@
  * Thiago Jung Bauermann <[email protected]>
  * Mimi Zohar <[email protected]>
  */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/seq_file.h>
 #include <linux/vmalloc.h>
security/integrity/ima/ima_main.c +0 -2
index 6e1576d9eb48..125a88d6eeca 100644
@@ -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>
security/integrity/ima/ima_policy.c +0 -2
index 453427048999..c334e0dc6083 100644
@@ -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>
security/integrity/ima/ima_queue.c +0 -2
index 1ce8b1701566..8753212ddb18 100644
@@ -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"
security/integrity/ima/ima_queue_keys.c +0 -2
index c87c72299191..cb3e3f501593 100644
@@ -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"
security/integrity/ima/ima_template.c +0 -2
index 6aa6408603e3..062d9ad49afb 100644
@@ -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"
security/integrity/ima/ima_template_lib.c +0 -2
index 32ae05d88257..9cd1e50f3ccc 100644
@@ -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)
security/integrity/integrity.h +6 -0
index 73fc286834d7..b1bb4d2263be 100644
@@ -6,6 +6,12 @@
  * Mimi Zohar <[email protected]>
  */
 
+#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>
Signature · 1 line
2.17.1
maintainer
#10 lore raw

Re: [PATCH v3 3/3] IMA: Add module name and base name prefix to log.

Quoted 12 lines · Tushar Sugandhi

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 <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
Suggested-by Shuah Khan <[email protected]>

<snip>

Quoted 7 lines
security/integrity/integrity.h +0 -0
index 73fc286834d7..b1bb4d2263be 100644
@@ -6,6 +6,12 @@
  * Mimi Zohar <[email protected]>
  */

security/integrity/integrity.h line 17

Quoted 10 lines
security/integrity/integrity.h +6 -0
 
+#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>

Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only affects the integrity directory but everything below it.  Adding KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM kernel messages.  Is that ok or should there be a separate pr_fmt() for the subdirectories?

thanks,

Mimi

maintainer
#11 lore raw

Re: [PATCH v3 3/3] IMA: Add module name and base name prefix to log.

Quoted 28 lines · Mimi Zohar
Quoted 15 lines · Tushar Sugandhi

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 <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
Suggested-by Shuah Khan <[email protected]>

<snip>

Quoted 9 lines
security/integrity/integrity.h +0 -0
index 73fc286834d7..b1bb4d2263be 100644
@@ -6,6 +6,12 @@
  * Mimi Zohar <[email protected]>
  */
Quoted 15 lines

security/integrity/integrity.h line 17

Quoted 9 lines
security/integrity/integrity.h +6 -0
+#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>

Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only affects the integrity directory but everything below it. Adding KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM kernel messages. Is that ok or should there be a separate pr_fmt() for the subdirectories?

Log messages are often consumed by log monitors, which mostly use pattern matching to find messages they're interested in, so you have to take some care when changing the messages the kernel spits out and you have to make sure any change gets well notified so the distributions can warn about it.

For this one, can we see a "before" and "after" message so we know what's happening?

James

#12 lore raw

Re: [PATCH v3 3/3] IMA: Add module name and base name prefix to log.

Quoted 8 lines · James Bottomley

Log messages are often consumed by log monitors, which mostly use pattern matching to find messages they're interested in, so you have to take some care when changing the messages the kernel spits out and you have to make sure any change gets well notified so the distributions can warn about it.

For this one, can we see a "before" and "after" message so we know what's happening?

https://patchwork.kernel.org/patch/11357335/#23134147

#13 lore raw

Re: [PATCH v3 3/3] IMA: Add module name and base name prefix to log.

In reply to James Bottomley
Quoted 46 lines · James Bottomley
Quoted 28 lines · Mimi Zohar
Quoted 15 lines · Tushar Sugandhi

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 <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
Suggested-by Shuah Khan <[email protected]>

<snip>

Quoted 9 lines
security/integrity/integrity.h +0 -0
index 73fc286834d7..b1bb4d2263be 100644
@@ -6,6 +6,12 @@
   * Mimi Zohar <[email protected]>
   */
Quoted 15 lines

security/integrity/integrity.h line 17

Quoted 9 lines
security/integrity/integrity.h +6 -0
+#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>

Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only affects the integrity directory but everything below it. Adding KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM kernel messages. Is that ok or should there be a separate pr_fmt() for the subdirectories?

Quoted 9 lines

Log messages are often consumed by log monitors, which mostly use pattern matching to find messages they're interested in, so you have to take some care when changing the messages the kernel spits out and you have to make sure any change gets well notified so the distributions can warn about it.

For this one, can we see a "before" and "after" message so we know what's happening?

Mimi and James,

My suggestion was based on thinking that simplifying this by removing duplicate defines. Some messages are missing modules names, adding module name to them does change the messages.

If using one pr_fmt for all modules changes the world and makes it difficult for log monitors, I would say it isn't a good change.

I will leave this totally up to Mimi to decide. Feel free to throw out my suggestion if it leads more trouble than help. :)

thanks,
-- Shuah

maintainer
#14 lore raw

Re: [PATCH v3 3/3] IMA: Add module name and base name prefix to log.

Quoted 63 lines · Shuah Khan
Quoted 46 lines · James Bottomley
Quoted 28 lines · Mimi Zohar
Quoted 15 lines · Tushar Sugandhi

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 <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
Suggested-by Shuah Khan <[email protected]>

<snip>

Quoted 9 lines
security/integrity/integrity.h +0 -0
index 73fc286834d7..b1bb4d2263be 100644
@@ -6,6 +6,12 @@
   * Mimi Zohar <[email protected]>
   */
Quoted 15 lines

security/integrity/integrity.h line 17

Quoted 9 lines
security/integrity/integrity.h +6 -0
+#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>

Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only affects the integrity directory but everything below it. Adding KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM kernel messages. Is that ok or should there be a separate pr_fmt() for the subdirectories?

Quoted 9 lines

Log messages are often consumed by log monitors, which mostly use pattern matching to find messages they're interested in, so you have to take some care when changing the messages the kernel spits out and you have to make sure any change gets well notified so the distributions can warn about it.

For this one, can we see a "before" and "after" message so we know what's happening?

Mimi and James,

My suggestion was based on thinking that simplifying this by removing duplicate defines. Some messages are missing modules names, adding module name to them does change the messages.

Quoted 5 lines

If using one pr_fmt for all modules changes the world and makes it difficult for log monitors, I would say it isn't a good change.

I will leave this totally up to Mimi to decide. Feel free to throw out my suggestion if it leads more trouble than help. :)

Thanks, Shuah.  Tushar, I don't see any need for changing the existing IMA/EVM messages.  Either remove the KBUILD_BASENAME from the format or limit the new format to the integrity directory.

thanks,

Mimi

author
#15 lore raw

Re: [PATCH v3 3/3] IMA: Add module name and base name prefix to log.

Quoted 67 lines · Mimi Zohar
Quoted 66 lines · Shuah Khan
Quoted 46 lines · James Bottomley
Quoted 28 lines · Mimi Zohar
Quoted 15 lines · Tushar Sugandhi

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 <[email protected]>
Reviewed-by Lakshmi Ramasubramanian <[email protected]>
Suggested-by Joe Perches <[email protected]>
Suggested-by Shuah Khan <[email protected]>

<snip>

Quoted 9 lines
security/integrity/integrity.h +0 -0
index 73fc286834d7..b1bb4d2263be 100644
@@ -6,6 +6,12 @@
    * Mimi Zohar <[email protected]>
    */
Quoted 15 lines

security/integrity/integrity.h line 17

Quoted 9 lines
security/integrity/integrity.h +6 -0
+#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>

Joe, Shuah, including the pr_fmt() in integrity/integrity.h not only affects the integrity directory but everything below it. Adding KBUILD_BASENAME to pr_fmt() modifies all of the existing IMA and EVM kernel messages. Is that ok or should there be a separate pr_fmt() for the subdirectories?

Quoted 9 lines

Log messages are often consumed by log monitors, which mostly use pattern matching to find messages they're interested in, so you have to take some care when changing the messages the kernel spits out and you have to make sure any change gets well notified so the distributions can warn about it.

For this one, can we see a "before" and "after" message so we know what's happening?

Mimi and James,

My suggestion was based on thinking that simplifying this by removing duplicate defines. Some messages are missing modules names, adding module name to them does change the messages.

If using one pr_fmt for all modules changes the world and makes it difficult for log monitors, I would say it isn't a good change.

Quoted 6 lines
Quoted 2 lines

I will leave this totally up to Mimi to decide. Feel free to throw out my suggestion if it leads more trouble than help. :)

Thanks, Shuah.  Tushar, I don't see any need for changing the existing IMA/EVM messages.  Either remove the KBUILD_BASENAME from the format or limit the new format to the integrity directory.

Ok. I will remove the KBUILD_BASENAME from the format.
And I will keep the definition in security/integrity/integrity.h, and will keep the duplicates removed - as originally proposed in this patch v3 3/3.

Quoted 5 lines

thanks,

Mimi

maintainer
#16 lore raw
In reply to Tushar Sugandhi

Hi Tushar,

"in IMA" is redundant in the above Subject line.

Quoted 11 lines · Tushar Sugandhi

Some files under IMA prefix the log statement with the respective file names and not with the string "ima". This is not consistent with the rest of the IMA files.

The function process_buffer_measurement() does not have log messages for failure conditions.

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

This patchset addresses the above issues.

The cover letter should provide a summary of the problem(s) being addressed by the individual patches, not a repetition of the individual patch descriptions.

Mimi

Quoted 26 lines

Tushar Sugandhi (3):
add log prefix to ima_mok.c, ima_asymmetric_keys.c, ima_queue_keys.c add log message to process_buffer_measurement failure conditions add module name and base name prefix to log statements

 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/Makefile              | 6 +++---
 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            | 5 +++--
 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 ++++++
 18 files changed, 12 insertions(+), 34 deletions(-)
author
#17 lore raw
Quoted 4 lines · Mimi Zohar

Hi Tushar,

"in IMA" is redundant in the above Subject line.

Thanks Mimi. I will fix it in the next iteration.

Quoted 8 lines · Tushar Sugandhi
Quoted 7 lines

Some files under IMA prefix the log statement with the respective file names and not with the string "ima". This is not consistent with the rest of the IMA files.

The function process_buffer_measurement() does not have log messages for failure conditions.

Quoted 9 lines
Quoted 4 lines

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

This patchset addresses the above issues.

The cover letter should provide a summary of the problem(s) being addressed by the individual patches, not a repetition of the individual patch descriptions.

Thanks. I will fix the cover letter description in the next iteration.

Quoted 28 lines

Mimi

Quoted 26 lines

Tushar Sugandhi (3):
add log prefix to ima_mok.c, ima_asymmetric_keys.c, ima_queue_keys.c add log message to process_buffer_measurement failure conditions add module name and base name prefix to log statements

  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/Makefile              | 6 +++---
  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            | 5 +++--
  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 ++++++
  18 files changed, 12 insertions(+), 34 deletions(-)