* [PATCH 0/2] security/keys/trusted: Fine-tuning for two function implementations @ 2017-11-10 20:28 SF Markus Elfring 2017-11-10 20:29 ` [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update() SF Markus Elfring 2017-11-10 20:30 ` [PATCH 2/2] KEYS: trusted: Use common error handling code in tpm_unseal() SF Markus Elfring 0 siblings, 2 replies; 5+ messages in thread From: SF Markus Elfring @ 2017-11-10 20:28 UTC (permalink / raw) To: keyrings, linux-integrity, linux-security-module, David Howells, James Morris, Mimi Zohar, Serge E. Hallyn Cc: LKML, kernel-janitors From: Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 10 Nov 2017 21:21:12 +0100 Two update suggestions were taken into account from static source code analysis. Markus Elfring (2): Use common error handling code in trusted_update() Use common error handling code in tpm_unseal() security/keys/trusted.c | 61 +++++++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 32 deletions(-) -- 2.15.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update() 2017-11-10 20:28 [PATCH 0/2] security/keys/trusted: Fine-tuning for two function implementations SF Markus Elfring @ 2017-11-10 20:29 ` SF Markus Elfring 2017-11-10 20:52 ` Julia Lawall 2017-11-10 20:30 ` [PATCH 2/2] KEYS: trusted: Use common error handling code in tpm_unseal() SF Markus Elfring 1 sibling, 1 reply; 5+ messages in thread From: SF Markus Elfring @ 2017-11-10 20:29 UTC (permalink / raw) To: keyrings, linux-integrity, linux-security-module, David Howells, James Morris, Mimi Zohar, Serge E. Hallyn Cc: LKML, kernel-janitors From: Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 10 Nov 2017 20:50:15 +0100 Adjust jump targets so that a bit of exception handling can be better reused at the end of this function. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- security/keys/trusted.c | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/security/keys/trusted.c b/security/keys/trusted.c index bd85315cbfeb..fd06d0c5323b 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -1078,30 +1078,18 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) if (!datablob) return -ENOMEM; new_o = trusted_options_alloc(); - if (!new_o) { - ret = -ENOMEM; - goto out; - } + if (!new_o) + goto e_nomem; + new_p = trusted_payload_alloc(key); - if (!new_p) { - ret = -ENOMEM; - goto out; - } + if (!new_p) + goto e_nomem; memcpy(datablob, prep->data, datalen); datablob[datalen] = '\0'; ret = datablob_parse(datablob, new_p, new_o); - if (ret != Opt_update) { - ret = -EINVAL; - kzfree(new_p); - goto out; - } - - if (!new_o->keyhandle) { - ret = -EINVAL; - kzfree(new_p); - goto out; - } + if (ret != Opt_update || !new_o->keyhandle) + goto e_inval; /* copy old key values, and reseal with new pcrs */ new_p->migratable = p->migratable; @@ -1113,23 +1101,31 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) ret = key_seal(new_p, new_o); if (ret < 0) { pr_info("trusted_key: key_seal failed (%d)\n", ret); - kzfree(new_p); - goto out; + goto free_payload; } if (new_o->pcrlock) { ret = pcrlock(new_o->pcrlock); if (ret < 0) { pr_info("trusted_key: pcrlock failed (%d)\n", ret); - kzfree(new_p); - goto out; + goto free_payload; } } rcu_assign_keypointer(key, new_p); call_rcu(&p->rcu, trusted_rcu_free); -out: +free_data: kzfree(datablob); kzfree(new_o); return ret; + +e_nomem: + ret = -ENOMEM; + goto free_data; + +e_inval: + ret = -EINVAL; +free_payload: + kzfree(new_p); + goto free_data; } /* -- 2.15.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update() 2017-11-10 20:29 ` [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update() SF Markus Elfring @ 2017-11-10 20:52 ` Julia Lawall 2017-11-11 9:37 ` SF Markus Elfring 0 siblings, 1 reply; 5+ messages in thread From: Julia Lawall @ 2017-11-10 20:52 UTC (permalink / raw) To: SF Markus Elfring Cc: keyrings, linux-integrity, linux-security-module, David Howells, James Morris, Mimi Zohar, Serge E. Hallyn, LKML, kernel-janitors On Fri, 10 Nov 2017, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 10 Nov 2017 20:50:15 +0100 > > Adjust jump targets so that a bit of exception handling can be better > reused at the end of this function. Unless there is a strong motivation for doing otherwise, the goal should be to make the code understandable and safe. Understandable means that issues specific to the error that occurred should be up at the place where the error occurs, ie any prints or any setting of return code. Safe means that cleanup code should appear once in a cascade at the end of the function, to minimize the chance that anything will be overlooked. Moving the ret assignments to the end of the function and adding the backward jumps doesn't make the code more understandable. A lot of mental effort is required to strace through the spaghetti code to find out what exactly will be the impact of a given failure. On the other hand, moving the kzalloc of new_p to the end of the function could be helpful, because it reduces the chance that new error handling code, if any turns out to be needed, will forget this operation. By why not just follow standard practice and free the structures in a cascade in the inverse of the order in which they are allocated at the end of the function? There can be a descriptive label for each thing that needs to be freed. julia > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > security/keys/trusted.c | 44 ++++++++++++++++++++------------------------ > 1 file changed, 20 insertions(+), 24 deletions(-) > > diff --git a/security/keys/trusted.c b/security/keys/trusted.c > index bd85315cbfeb..fd06d0c5323b 100644 > --- a/security/keys/trusted.c > +++ b/security/keys/trusted.c > @@ -1078,30 +1078,18 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) > if (!datablob) > return -ENOMEM; > new_o = trusted_options_alloc(); > - if (!new_o) { > - ret = -ENOMEM; > - goto out; > - } > + if (!new_o) > + goto e_nomem; > + > new_p = trusted_payload_alloc(key); > - if (!new_p) { > - ret = -ENOMEM; > - goto out; > - } > + if (!new_p) > + goto e_nomem; > > memcpy(datablob, prep->data, datalen); > datablob[datalen] = '\0'; > ret = datablob_parse(datablob, new_p, new_o); > - if (ret != Opt_update) { > - ret = -EINVAL; > - kzfree(new_p); > - goto out; > - } > - > - if (!new_o->keyhandle) { > - ret = -EINVAL; > - kzfree(new_p); > - goto out; > - } > + if (ret != Opt_update || !new_o->keyhandle) > + goto e_inval; > > /* copy old key values, and reseal with new pcrs */ > new_p->migratable = p->migratable; > @@ -1113,23 +1101,31 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) > ret = key_seal(new_p, new_o); > if (ret < 0) { > pr_info("trusted_key: key_seal failed (%d)\n", ret); > - kzfree(new_p); > - goto out; > + goto free_payload; > } > if (new_o->pcrlock) { > ret = pcrlock(new_o->pcrlock); > if (ret < 0) { > pr_info("trusted_key: pcrlock failed (%d)\n", ret); > - kzfree(new_p); > - goto out; > + goto free_payload; > } > } > rcu_assign_keypointer(key, new_p); > call_rcu(&p->rcu, trusted_rcu_free); > -out: > +free_data: > kzfree(datablob); > kzfree(new_o); > return ret; > + > +e_nomem: > + ret = -ENOMEM; > + goto free_data; > + > +e_inval: > + ret = -EINVAL; > +free_payload: > + kzfree(new_p); > + goto free_data; > } > > /* > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: KEYS: trusted: Use common error handling code in trusted_update() 2017-11-10 20:52 ` Julia Lawall @ 2017-11-11 9:37 ` SF Markus Elfring 0 siblings, 0 replies; 5+ messages in thread From: SF Markus Elfring @ 2017-11-11 9:37 UTC (permalink / raw) To: Julia Lawall, keyrings, linux-integrity, linux-security-module Cc: David Howells, James Morris, Mimi Zohar, Serge E. Hallyn, LKML, kernel-janitors > Safe means that cleanup code should appear once in a cascade at the end > of the function, to minimize the chance that anything will be overlooked. I find that the control flow of this function implementation does not fit to the mentioned ideal so far. > Moving the ret assignments to the end of the function and adding the > backward jumps doesn't make the code more understandable. Is this structure required if you would like to achieve something in the shown software design direction? > On the other hand, moving the kzalloc of new_p to the end of the function > could be helpful, Why do you think that the movement of this function call can finally work in the concrete software situation? > because it reduces the chance that new error handling code, > if any turns out to be needed, will forget this operation. Your expectation can be nice. > By why not just follow standard practice and free the structures in a > cascade in the inverse of the order in which they are allocated at the end > of the function? This is still happening here partly, isn't it? > There can be a descriptive label for each thing that needs to be freed. Which identifiers would you find more appropriate in comparison to my suggestion? * e_inval * e_nomem * free_data * free_payload Regards, Markus ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] KEYS: trusted: Use common error handling code in tpm_unseal() 2017-11-10 20:28 [PATCH 0/2] security/keys/trusted: Fine-tuning for two function implementations SF Markus Elfring 2017-11-10 20:29 ` [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update() SF Markus Elfring @ 2017-11-10 20:30 ` SF Markus Elfring 1 sibling, 0 replies; 5+ messages in thread From: SF Markus Elfring @ 2017-11-10 20:30 UTC (permalink / raw) To: keyrings, linux-integrity, linux-security-module, David Howells, James Morris, Mimi Zohar, Serge E. Hallyn Cc: LKML, kernel-janitors From: Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 10 Nov 2017 21:15:53 +0100 * Add a jump target so that a specific error message is stored only once at the end of this function implementation. * Replace two calls of the macro "pr_info" by goto statements. * Adjust two condition checks. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- security/keys/trusted.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/security/keys/trusted.c b/security/keys/trusted.c index fd06d0c5323b..a295eaae40ba 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -591,15 +591,12 @@ static int tpm_unseal(struct tpm_buf *tb, /* sessions for unsealing key and data */ ret = oiap(tb, &authhandle1, enonce1); - if (ret < 0) { - pr_info("trusted_key: oiap failed (%d)\n", ret); - return ret; - } + if (ret) + goto report_failure; + ret = oiap(tb, &authhandle2, enonce2); - if (ret < 0) { - pr_info("trusted_key: oiap failed (%d)\n", ret); - return ret; - } + if (ret) + goto report_failure; ordinal = htonl(TPM_ORD_UNSEAL); keyhndl = htonl(SRKHANDLE); @@ -654,6 +651,10 @@ static int tpm_unseal(struct tpm_buf *tb, } memcpy(data, tb->data + TPM_DATA_OFFSET + sizeof(uint32_t), *datalen); return 0; + +report_failure: + pr_info("trusted_key: oiap failed (%d)\n", ret); + return ret; } /* -- 2.15.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-11 9:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-10 20:28 [PATCH 0/2] security/keys/trusted: Fine-tuning for two function implementations SF Markus Elfring 2017-11-10 20:29 ` [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update() SF Markus Elfring 2017-11-10 20:52 ` Julia Lawall 2017-11-11 9:37 ` SF Markus Elfring 2017-11-10 20:30 ` [PATCH 2/2] KEYS: trusted: Use common error handling code in tpm_unseal() SF Markus Elfring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).