From: Wei Yongjun <[email protected]>
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
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)
On Wed, Jul 15, 2020 at 11:28:38PM +0100, David Howells wrote:
> From: Wei Yongjun <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: David Howells <[email protected]>
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: David Howells <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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/[email protected]/
/Jarkko