* [PATCH] keys: asymmetric: fix error return code in software_key_query()
@ 2020-07-15 22:28 David Howells
2020-07-23 1:32 ` Jarkko Sakkinen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: David Howells @ 2020-07-15 22:28 UTC (permalink / raw)
To: torvalds
Cc: Wei Yongjun, dhowells, jarkko.sakkinen, keyrings,
linux-security-module, linux-kernel
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(+)
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
2020-07-15 22:28 [PATCH] keys: asymmetric: fix error return code in software_key_query() David Howells
@ 2020-07-23 1:32 ` Jarkko Sakkinen
2020-07-23 1:36 ` Jarkko Sakkinen
2020-07-23 7:31 ` David Howells
2020-07-23 7:42 ` David Howells
2 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-07-23 1:32 UTC (permalink / raw)
To: David Howells
Cc: torvalds, Wei Yongjun, keyrings, linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
2020-07-23 1:32 ` Jarkko Sakkinen
@ 2020-07-23 1:36 ` Jarkko Sakkinen
0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-07-23 1:36 UTC (permalink / raw)
To: David Howells
Cc: torvalds, Wei Yongjun, keyrings, linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
2020-07-15 22:28 [PATCH] keys: asymmetric: fix error return code in software_key_query() David Howells
2020-07-23 1:32 ` Jarkko Sakkinen
@ 2020-07-23 7:31 ` David Howells
2020-07-23 7:42 ` David Howells
2 siblings, 0 replies; 6+ messages in thread
From: David Howells @ 2020-07-23 7:31 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dhowells, torvalds, Wei Yongjun, keyrings, linux-security-module,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
2020-07-15 22:28 [PATCH] keys: asymmetric: fix error return code in software_key_query() David Howells
2020-07-23 1:32 ` Jarkko Sakkinen
2020-07-23 7:31 ` David Howells
@ 2020-07-23 7:42 ` David Howells
2020-07-24 7:16 ` Jarkko Sakkinen
2 siblings, 1 reply; 6+ messages in thread
From: David Howells @ 2020-07-23 7:42 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dhowells, torvalds, Wei Yongjun, keyrings, linux-security-module,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] keys: asymmetric: fix error return code in software_key_query()
2020-07-23 7:42 ` David Howells
@ 2020-07-24 7:16 ` Jarkko Sakkinen
0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-07-24 7:16 UTC (permalink / raw)
To: David Howells
Cc: torvalds, Wei Yongjun, keyrings, linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-24 7:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 22:28 [PATCH] keys: asymmetric: fix error return code in software_key_query() David Howells
2020-07-23 1:32 ` Jarkko Sakkinen
2020-07-23 1:36 ` Jarkko Sakkinen
2020-07-23 7:31 ` David Howells
2020-07-23 7:42 ` David Howells
2020-07-24 7:16 ` Jarkko Sakkinen
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).