diff mbox series

keys: asymmetric: fix error return code in software_key_query()

Message ID 159485211858.2340757.9890754969922775496.stgit@warthog.procyon.org.uk (mailing list archive)
State New, archived
Headers show
Series keys: asymmetric: fix error return code in software_key_query() | expand

Commit Message

David Howells July 15, 2020, 10:28 p.m. UTC
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>
---

 crypto/asymmetric_keys/public_key.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Jarkko Sakkinen July 23, 2020, 1:32 a.m. UTC | #1
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
Jarkko Sakkinen July 23, 2020, 1:36 a.m. UTC | #2
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
David Howells July 23, 2020, 7:31 a.m. UTC | #3
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
David Howells July 23, 2020, 7:42 a.m. UTC | #4
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
Jarkko Sakkinen July 24, 2020, 7:16 a.m. UTC | #5
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 mbox series

Patch

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)