Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
On Thu, Jul 23, 2020 at 08:42:25AM +0100, David Howells wrote: > Jarkko Sakkinen 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.sakki...@linux.intel.com/ /Jarkko
Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
Jarkko Sakkinen 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
Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
Jarkko Sakkinen 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
Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
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
> >
> > 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
> > Signed-off-by: David Howells
>
> 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
Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
On Wed, Jul 15, 2020 at 11:28:38PM +0100, David Howells wrote:
> From: Wei Yongjun
>
> 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
> Signed-off-by: David Howells
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
[PATCH] keys: asymmetric: fix error return code in software_key_query()
From: Wei Yongjun
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
Signed-off-by: David Howells
---
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;
key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
GFP_KERNEL);
if (!key)
