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(-)
--
2.17.1
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(-)
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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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 <[email protected]>
* Mimi Zohar <[email protected]>
*/
-#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 6e1576d9eb48..125a88d6eeca 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 c87c72299191..cb3e3f501593 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 <[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>
--
2.17.1
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(+)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9fe949c6a530..6e1576d9eb48 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("%s: failed, result: %d\n", __func__, ret);
+
return;
}
--
2.17.1
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(-)
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
On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
> 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>
> 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 <[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>
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
On Tue, 2020-02-11 at 15:14 -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".
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
>
> 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(-)
>
> 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
Hi Tushar,
Please remove the period at the end of the Subject line.
On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
> 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.
>
> 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(+)
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 9fe949c6a530..6e1576d9eb48 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("%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
Hi Tushar,
"in IMA" is redundant in the above Subject line.
On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
> 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
>
> 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(-)
>
On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote:
> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
> > 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>
>
> > 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 <[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>
>
> 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
On Wed, 2020-02-12 at 10:26 -0500, James Bottomley wrote:
> 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
On 2020-02-12 7:23 a.m., Mimi Zohar wrote:
> Hi Tushar,
>
> "in IMA" is redundant in the above Subject line.
>
Thanks Mimi. I will fix it in the next iteration.
> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
>> 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.
>
Thanks. I will fix the cover letter description in the next iteration.
> Mimi
>
>>
>> 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(-)
>>
On 2020-02-12 6:49 a.m., Mimi Zohar wrote:
> On Tue, 2020-02-11 at 15:14 -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".
>
> 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.
> Mimi
>
> [1] Refer to Documentation/kbuild/makefiles.rst
>
>>
>> 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(-)
>>
>> 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
On 2020-02-12 6:47 a.m., Mimi Zohar wrote:
> Hi Tushar,
>
> Please remove the period at the end of the Subject line.
Thanks. I will fix it in the next iteration.
>
> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
>> 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.
>
>>
>> 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(+)
>>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 9fe949c6a530..6e1576d9eb48 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("%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.
> Mimi
>
On 2/12/20 8:26 AM, James Bottomley wrote:
> On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote:
>> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
>>> 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>
>>
>>> 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 <[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>
>>
>> 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?
>
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
On Wed, 2020-02-12 at 14:30 -0800, Tushar Sugandhi wrote:
>
> On 2020-02-12 6:47 a.m., Mimi Zohar wrote:
> > Hi Tushar,
> >
> > Please remove the period at the end of the Subject line.
> Thanks. I will fix it in the next iteration.
> >
> > On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
> >> 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.
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
On Wed, 2020-02-12 at 15:52 -0700, Shuah Khan wrote:
> On 2/12/20 8:26 AM, James Bottomley wrote:
> > On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote:
> >> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
> >>> 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>
> >>
> >>> 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 <[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>
> >>
> >> 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?
> >
>
> 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. 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
On 2020-02-12 4:38 p.m., Mimi Zohar wrote:
> On Wed, 2020-02-12 at 15:52 -0700, Shuah Khan wrote:
>> On 2/12/20 8:26 AM, James Bottomley wrote:
>>> On Wed, 2020-02-12 at 09:29 -0500, Mimi Zohar wrote:
>>>> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
>>>>> 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>
>>>>
>>>>> 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 <[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>
>>>>
>>>> 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?
>>>
>>
>> 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. 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.
>
> thanks,
>
> Mimi
>
On 2020-02-12 4:21 p.m., Mimi Zohar wrote:
> On Wed, 2020-02-12 at 14:30 -0800, Tushar Sugandhi wrote:
>>
>> On 2020-02-12 6:47 a.m., Mimi Zohar wrote:
>>> Hi Tushar,
>>>
>>> Please remove the period at the end of the Subject line.
>> Thanks. I will fix it in the next iteration.
>>>
>>> On Tue, 2020-02-11 at 15:14 -0800, Tushar Sugandhi wrote:
>>>> 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.
>
> 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.
>
> Mimi
>