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
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 <[email protected]>
Reviewed-by: Lakshmi Ramasubramanian <[email protected]>
---
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
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 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 <[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
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
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;
}
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
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
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
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.