| Message ID | 159485211858.2340757.9890754969922775496.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | keys: asymmetric: fix error return code in software_key_query() | expand |
On Wed, Jul 15, 2020 at 11:28:38PM +0100, David Howells wrote: > From: Wei Yongjun <weiyongjun1@huawei.com> > > Fix to return negative error code -ENOMEM from kmalloc() error handling > case instead of 0, as done elsewhere in this function. > > Fixes: f1774cb8956a ("X.509: parse public key parameters from x509 for akcipher") > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > Signed-off-by: David Howells <dhowells@redhat.com> Why f1774cb8956a lacked any possible testing? It extends ABI anyway. I think it is a kind of change that would require more screening before getting applied. > --- > > crypto/asymmetric_keys/public_key.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > index d7f43d4ea925..e5fae4e838c0 100644 > --- a/crypto/asymmetric_keys/public_key.c > +++ b/crypto/asymmetric_keys/public_key.c > @@ -119,6 +119,7 @@ static int software_key_query(const struct kernel_pkey_params *params, > if (IS_ERR(tfm)) > return PTR_ERR(tfm); > > + ret = -ENOMEM; This is extremely confusing to read way to handle 'ret'. Would be way more cleaner to be just simple and stupid: if (!key) { ret = -ENOMEM; goto error_free_tfm; } > key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, > GFP_KERNEL); > if (!key) /Jarkko
On Thu, Jul 23, 2020 at 04:32:38AM +0300, Jarkko Sakkinen wrote: > On Wed, Jul 15, 2020 at 11:28:38PM +0100, David Howells wrote: > > From: Wei Yongjun <weiyongjun1@huawei.com> > > > > Fix to return negative error code -ENOMEM from kmalloc() error handling > > case instead of 0, as done elsewhere in this function. > > > > Fixes: f1774cb8956a ("X.509: parse public key parameters from x509 for akcipher") > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > Signed-off-by: David Howells <dhowells@redhat.com> > > Why f1774cb8956a lacked any possible testing? It extends ABI anyway. > > I think it is a kind of change that would require more screening before > getting applied. > > > --- > > > > crypto/asymmetric_keys/public_key.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > > index d7f43d4ea925..e5fae4e838c0 100644 > > --- a/crypto/asymmetric_keys/public_key.c > > +++ b/crypto/asymmetric_keys/public_key.c > > @@ -119,6 +119,7 @@ static int software_key_query(const struct kernel_pkey_params *params, > > if (IS_ERR(tfm)) > > return PTR_ERR(tfm); > > > > + ret = -ENOMEM; > > This is extremely confusing to read way to handle 'ret'. > > Would be way more cleaner to be just simple and stupid: > > if (!key) { > ret = -ENOMEM; > goto error_free_tfm; > } To rationalize why the 2nd way is better: the diff would tell the whole story. Now this commit requires to check *both* the diff and the source file to get the full understanding what is going on. /Jarkko
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > if (IS_ERR(tfm)) > > return PTR_ERR(tfm); > > > > + ret = -ENOMEM; > > This is extremely confusing to read way to handle 'ret'. > > Would be way more cleaner to be just simple and stupid: > > if (!key) { > ret = -ENOMEM; > goto error_free_tfm; > } I agree, but we have some people who will (or who used to) moan at you for doing in four lines what you could've done in three. I don't know if this is still the standard. David
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > Why f1774cb8956a lacked any possible testing? It extends ABI anyway. > > I think it is a kind of change that would require more screening before > getting applied. Yeah. It went in via a round-about route. I left off development of it when the tpm stuff I wrote broke because the tpm2 stuff went in upstream. I then handed the patches off to Denis who did the tpm support, but I never got my stuff finished enough to work out how to do the testsuite (since it would involve using a tpm). However, since I did the PKCS#8 testing module as well, I guess I don't need that to at least test the API. I'll look at using that to add some tests. Any suggestions as to how to do testing via the tpm? David
On Thu, Jul 23, 2020 at 08:42:25AM +0100, David Howells wrote: > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > > Why f1774cb8956a lacked any possible testing? It extends ABI anyway. > > > > I think it is a kind of change that would require more screening before > > getting applied. > > Yeah. It went in via a round-about route. I left off development of it when > the tpm stuff I wrote broke because the tpm2 stuff went in upstream. I then > handed the patches off to Denis who did the tpm support, but I never got my > stuff finished enough to work out how to do the testsuite (since it would > involve using a tpm). However, since I did the PKCS#8 testing module as well, > I guess I don't need that to at least test the API. I'll look at using that > to add some tests. Any suggestions as to how to do testing via the tpm? > > David The unfortunate thing is that I was not involved with asym_tpm.c review process in any possible way, which means that at the moment I lack both: 1. Knowledge of crypto/asymmetric_keys. 2. How asym_tpm.c is implemented. I only became aware of asym_tpm.c's existence last Sep [*]. I'll put to my backlog to try TPM asymmetric keys (earliest when I'm back from vacation 08/10). [*] https://lore.kernel.org/linux-integrity/20190926171601.30404-1-jarkko.sakkinen@linux.intel.com/ /Jarkko
diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index d7f43d4ea925..e5fae4e838c0 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -119,6 +119,7 @@ static int software_key_query(const struct kernel_pkey_params *params, if (IS_ERR(tfm)) return PTR_ERR(tfm); + ret = -ENOMEM; key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, GFP_KERNEL); if (!key)