| Message ID | 20200211024755.5579-2-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] IMA: Update KBUILD_MODNAME for IMA files to ima | expand |
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; }
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.
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; }