IMA: Update KBUILD_MODNAME for IMA files to ima

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

Quoted 2 lines · Tushar Sugandhi

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

trivia:

security/integrity/ima/ima_main.c

Quoted 1 line

security/integrity/ima/ima_main.c +0 -0

[]

Quoted 3 lines

security/integrity/ima/ima_main.c +0 -0

@@ -757,6 +757,9 @@ void process_buffer_measurement(const void *buf, int size,
  		ima_free_template_entry(entry);
 

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

Quoted 3 lines

security/integrity/ima/ima_main.c +2 -0

  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);

security/integrity/ima/ima_queue_keys.c

Quoted 1 line

security/integrity/ima/ima_queue_keys.c +0 -0

[]

Quoted 2 lines

security/integrity/ima/ima_queue_keys.c +0 -0

@@ -90,6 +90,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
 

security/integrity/ima/ima_queue_keys.c static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring, · line 96

Quoted 6 lines

security/integrity/ima/ima_queue_keys.c +1 -0

  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(-)

security/integrity/ima/ima_queue_keys.c +0 -3

index c87c722..ba449f 100644
@@ -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);

security/integrity/ima/ima_queue_keys.c +6 -16

  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) {

Hi Joe,

Quoted 3 lines · Tushar Sugandhi

Quoted 2 lines

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

Quoted 4 lines

security/integrity/ima/ima_queue_keys.c

Quoted 1 line

security/integrity/ima/ima_queue_keys.c +0 -0

[]

Quoted 2 lines

security/integrity/ima/ima_queue_keys.c +0 -0

@@ -90,6 +90,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
 

Quoted 9 lines

security/integrity/ima/ima_queue_keys.c static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring, · line 96

Quoted 6 lines

security/integrity/ima/ima_queue_keys.c +1 -0

  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.

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?

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.

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

Hi Joe,

Quoted 6 lines

Series seems sensible, thanks Tushar.

Next time you might choose to use

    git format-patch --cover-letter

Quoted 3 lines

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