Jonathan suggests adding cleanup.h support for x509_certificate structs:
https://lore.kernel.org/all/[email protected]/
Introduce a DEFINE_FREE() macro and use it in x509_cert_parse() and
x509_key_preparse(). These are the only functions where scope-based
x509_certificate allocation currently makes sense. Another user will
be introduced with the upcoming SPDM library (Security Protocol and
Data Model) for PCI device authentication.
Unlike most other DEFINE_FREE() macros, this one not only has to check
for NULL, but also for ERR_PTR() before calling x509_free_certificate()
at end of scope. That's because the "constructor" of x509_certificate
structs, x509_cert_parse(), may return an ERR_PTR().
I've compared the Assembler output before/after and while I couldn't
spot a functional difference, I did notice an annoying superfluous check
being added to each function:
* x509_key_preparse() now checks that "cert" is not NULL before calling
x509_free_certificate() at end of scope. It knows whether "cert" is
an ERR_PTR() because of the explicit "if (IS_ERR(cert))" check at the
top of the function, but it doesn't know whether it's NULL. In fact
it can *never* be NULL because x509_cert_parse() only returns either
a valid pointer or an ERR_PTR().
I've tried adding __attribute__((returns_nonnull)) to x509_cert_parse()
but the compiler ignores it due to commit a3ca86aea507
("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS").
* x509_cert_parse() now checks that "cert" is not an ERR_PTR() before
calling x509_free_certificate() at end of scope. The compiler doesn't
know that kzalloc() never returns an ERR_PTR().
I've tried telling the compiler that by amending kmalloc() with
"if (IS_ERR(ptr)) __builtin_unreachable();", but the result was
disappointing: While it succeeded in eliminating the superfluous
ERR_PTR() check, total vmlinux size increased by 448 bytes.
I could add such a clause locally to x509_cert_parse() instead of
kmalloc(), but it would require additions to compiler-*.h.
(clang uses a different syntax for these annotations.)
Despite the annoying extra checks, I think the gain in readability
justifies the conversion.
Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
crypto/asymmetric_keys/x509_cert_parser.c | 42 +++++++++++--------------------
crypto/asymmetric_keys/x509_parser.h | 3 +++
crypto/asymmetric_keys/x509_public_key.c | 31 +++++++----------------
3 files changed, 27 insertions(+), 49 deletions(-)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 487204d..e597ac6 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -60,24 +60,23 @@ void x509_free_certificate(struct x509_certificate *cert)
*/
struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
{
- struct x509_certificate *cert;
- struct x509_parse_context *ctx;
+ struct x509_certificate *cert __free(x509_free_certificate);
+ struct x509_parse_context *ctx __free(kfree) = NULL;
struct asymmetric_key_id *kid;
long ret;
- ret = -ENOMEM;
cert = kzalloc(sizeof(struct x509_certificate), GFP_KERNEL);
if (!cert)
- goto error_no_cert;
+ return ERR_PTR(-ENOMEM);
cert->pub = kzalloc(sizeof(struct public_key), GFP_KERNEL);
if (!cert->pub)
- goto error_no_ctx;
+ return ERR_PTR(-ENOMEM);
cert->sig = kzalloc(sizeof(struct public_key_signature), GFP_KERNEL);
if (!cert->sig)
- goto error_no_ctx;
+ return ERR_PTR(-ENOMEM);
ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);
if (!ctx)
- goto error_no_ctx;
+ return ERR_PTR(-ENOMEM);
ctx->cert = cert;
ctx->data = (unsigned long)data;
@@ -85,7 +84,7 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
/* Attempt to decode the certificate */
ret = asn1_ber_decoder(&x509_decoder, ctx, data, datalen);
if (ret < 0)
- goto error_decode;
+ return ERR_PTR(ret);
/* Decode the AuthorityKeyIdentifier */
if (ctx->raw_akid) {
@@ -95,20 +94,19 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
ctx->raw_akid, ctx->raw_akid_size);
if (ret < 0) {
pr_warn("Couldn't decode AuthKeyIdentifier\n");
- goto error_decode;
+ return ERR_PTR(ret);
}
}
- ret = -ENOMEM;
cert->pub->key = kmemdup(ctx->key, ctx->key_size, GFP_KERNEL);
if (!cert->pub->key)
- goto error_decode;
+ return ERR_PTR(-ENOMEM);
cert->pub->keylen = ctx->key_size;
cert->pub->params = kmemdup(ctx->params, ctx->params_size, GFP_KERNEL);
if (!cert->pub->params)
- goto error_decode;
+ return ERR_PTR(-ENOMEM);
cert->pub->paramlen = ctx->params_size;
cert->pub->algo = ctx->key_algo;
@@ -116,33 +114,23 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
/* Grab the signature bits */
ret = x509_get_sig_params(cert);
if (ret < 0)
- goto error_decode;
+ return ERR_PTR(ret);
/* Generate cert issuer + serial number key ID */
kid = asymmetric_key_generate_id(cert->raw_serial,
cert->raw_serial_size,
cert->raw_issuer,
cert->raw_issuer_size);
- if (IS_ERR(kid)) {
- ret = PTR_ERR(kid);
- goto error_decode;
- }
+ if (IS_ERR(kid))
+ return ERR_CAST(kid);
cert->id = kid;
/* Detect self-signed certificates */
ret = x509_check_for_self_signed(cert);
if (ret < 0)
- goto error_decode;
-
- kfree(ctx);
- return cert;
+ return ERR_PTR(ret);
-error_decode:
- kfree(ctx);
-error_no_ctx:
- x509_free_certificate(cert);
-error_no_cert:
- return ERR_PTR(ret);
+ return_ptr(cert);
}
EXPORT_SYMBOL_GPL(x509_cert_parse);
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 97a886c..d2dfe50 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -5,6 +5,7 @@
* Written by David Howells ([email protected])
*/
+#include <linux/cleanup.h>
#include <linux/time.h>
#include <crypto/public_key.h>
#include <keys/asymmetric-type.h>
@@ -44,6 +45,8 @@ struct x509_certificate {
* x509_cert_parser.c
*/
extern void x509_free_certificate(struct x509_certificate *cert);
+DEFINE_FREE(x509_free_certificate, struct x509_certificate *,
+ if (!IS_ERR_OR_NULL(_T)) x509_free_certificate(_T))
extern struct x509_certificate *x509_cert_parse(const void *data, size_t datalen);
extern int x509_decode_time(time64_t *_t, size_t hdrlen,
unsigned char tag,
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 6a4f00b..00ac715 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -161,12 +161,11 @@ int x509_check_for_self_signed(struct x509_certificate *cert)
*/
static int x509_key_preparse(struct key_preparsed_payload *prep)
{
- struct asymmetric_key_ids *kids;
- struct x509_certificate *cert;
+ struct x509_certificate *cert __free(x509_free_certificate);
+ struct asymmetric_key_ids *kids __free(kfree) = NULL;
+ char *p, *desc __free(kfree) = NULL;
const char *q;
size_t srlen, sulen;
- char *desc = NULL, *p;
- int ret;
cert = x509_cert_parse(prep->data, prep->datalen);
if (IS_ERR(cert))
@@ -188,9 +187,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
}
/* Don't permit addition of blacklisted keys */
- ret = -EKEYREJECTED;
if (cert->blacklisted)
- goto error_free_cert;
+ return -EKEYREJECTED;
/* Propose a description */
sulen = strlen(cert->subject);
@@ -202,10 +200,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
q = cert->raw_serial;
}
- ret = -ENOMEM;
desc = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
if (!desc)
- goto error_free_cert;
+ return -ENOMEM;
p = memcpy(desc, cert->subject, sulen);
p += sulen;
*p++ = ':';
@@ -215,16 +212,14 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
kids = kmalloc(sizeof(struct asymmetric_key_ids), GFP_KERNEL);
if (!kids)
- goto error_free_desc;
+ return -ENOMEM;
kids->id[0] = cert->id;
kids->id[1] = cert->skid;
kids->id[2] = asymmetric_key_generate_id(cert->raw_subject,
cert->raw_subject_size,
"", 0);
- if (IS_ERR(kids->id[2])) {
- ret = PTR_ERR(kids->id[2]);
- goto error_free_kids;
- }
+ if (IS_ERR(kids->id[2]))
+ return PTR_ERR(kids->id[2]);
/* We're pinning the module by being linked against it */
__module_get(public_key_subtype.owner);
@@ -242,15 +237,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
cert->sig = NULL;
desc = NULL;
kids = NULL;
- ret = 0;
-
-error_free_kids:
- kfree(kids);
-error_free_desc:
- kfree(desc);
-error_free_cert:
- x509_free_certificate(cert);
- return ret;
+ return 0;
}
static struct asymmetric_key_parser x509_key_parser = {
--
2.40.1
On Sun Jan 21, 2024 at 7:50 PM EET, Lukas Wunner wrote:
> Jonathan suggests adding cleanup.h support for x509_certificate structs:
> https://lore.kernel.org/all/[email protected]/
You have suggested-by. Use link/closes/whatever tag if you want to link
that message (and rip off this paragraph). Since the whole feature is
new maybe it would make sense to remind about __cleanup__ attribute and
its use or at least link: https://lwn.net/Articles/934679/.
You are writing it like "every knows this already".
> Introduce a DEFINE_FREE() macro and use it in x509_cert_parse() and
> x509_key_preparse(). These are the only functions where scope-based
DEFINE_FREE() macro already exists. This patch just invokes
the already existing macro.
> x509_certificate allocation currently makes sense. Another user will
> be introduced with the upcoming SPDM library (Security Protocol and
> Data Model) for PCI device authentication.
What is it and why we care about it here?
>
> Unlike most other DEFINE_FREE() macros, this one not only has to check
So they are not DEFINE_FREE() macros but locations that invoke
DEFINE_FREE() (i.e. DEFINE_FREE9() invocations).
> for NULL, but also for ERR_PTR() before calling x509_free_certificate()
> at end of scope. That's because the "constructor" of x509_certificate
> structs, x509_cert_parse(), may return an ERR_PTR().
"Unlike usual DEFINE_FREE() invocations, return value may also contain
error, matching the possible return values of x509_cert_parse()."
> I've compared the Assembler output before/after and while I couldn't
> spot a functional difference, I did notice an annoying superfluous check
> being added to each function:
The tail of the commit message looks more like diary, notes, plans than
anything senseful for a commit message.
> x509_free_certificate() at end of scope. It knows whether "cert" is
> an ERR_PTR() because of the explicit "if (IS_ERR(cert))" check at the
> top of the function, but it doesn't know whether it's NULL. In fact
> it can *never* be NULL because x509_cert_parse() only returns either
> a valid pointer or an ERR_PTR().
>
> I've tried adding __attribute__((returns_nonnull)) to x509_cert_parse()
> but the compiler ignores it due to commit a3ca86aea507
> ("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS").
>
> * x509_cert_parse() now checks that "cert" is not an ERR_PTR() before
> calling x509_free_certificate() at end of scope. The compiler doesn't
> know that kzalloc() never returns an ERR_PTR().
>
> I've tried telling the compiler that by amending kmalloc() with
> "if (IS_ERR(ptr)) __builtin_unreachable();", but the result was
> disappointing: While it succeeded in eliminating the superfluous
> ERR_PTR() check, total vmlinux size increased by 448 bytes.
>
> I could add such a clause locally to x509_cert_parse() instead of
> kmalloc(), but it would require additions to compiler-*.h.
> (clang uses a different syntax for these annotations.)
>
> Despite the annoying extra checks, I think the gain in readability
> justifies the conversion.
>
> Suggested-by: Jonathan Cameron <[email protected]>
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
Maybe you want to put tail here.
> crypto/asymmetric_keys/x509_cert_parser.c | 42 +++++++++++--------------------
> crypto/asymmetric_keys/x509_parser.h | 3 +++
> crypto/asymmetric_keys/x509_public_key.c | 31 +++++++----------------
> 3 files changed, 27 insertions(+), 49 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 487204d..e597ac6 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -60,24 +60,23 @@ void x509_free_certificate(struct x509_certificate *cert)
> */
> struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> {
> - struct x509_certificate *cert;
> - struct x509_parse_context *ctx;
> + struct x509_certificate *cert __free(x509_free_certificate);
> + struct x509_parse_context *ctx __free(kfree) = NULL;
> struct asymmetric_key_id *kid;
> long ret;
>
> - ret = -ENOMEM;
> cert = kzalloc(sizeof(struct x509_certificate), GFP_KERNEL);
> if (!cert)
> - goto error_no_cert;
> + return ERR_PTR(-ENOMEM);
> cert->pub = kzalloc(sizeof(struct public_key), GFP_KERNEL);
> if (!cert->pub)
> - goto error_no_ctx;
> + return ERR_PTR(-ENOMEM);
> cert->sig = kzalloc(sizeof(struct public_key_signature), GFP_KERNEL);
> if (!cert->sig)
> - goto error_no_ctx;
> + return ERR_PTR(-ENOMEM);
> ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);
> if (!ctx)
> - goto error_no_ctx;
> + return ERR_PTR(-ENOMEM);
>
> ctx->cert = cert;
> ctx->data = (unsigned long)data;
> @@ -85,7 +84,7 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> /* Attempt to decode the certificate */
> ret = asn1_ber_decoder(&x509_decoder, ctx, data, datalen);
> if (ret < 0)
> - goto error_decode;
> + return ERR_PTR(ret);
>
> /* Decode the AuthorityKeyIdentifier */
> if (ctx->raw_akid) {
> @@ -95,20 +94,19 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> ctx->raw_akid, ctx->raw_akid_size);
> if (ret < 0) {
> pr_warn("Couldn't decode AuthKeyIdentifier\n");
> - goto error_decode;
> + return ERR_PTR(ret);
> }
> }
>
> - ret = -ENOMEM;
> cert->pub->key = kmemdup(ctx->key, ctx->key_size, GFP_KERNEL);
> if (!cert->pub->key)
> - goto error_decode;
> + return ERR_PTR(-ENOMEM);
>
> cert->pub->keylen = ctx->key_size;
>
> cert->pub->params = kmemdup(ctx->params, ctx->params_size, GFP_KERNEL);
> if (!cert->pub->params)
> - goto error_decode;
> + return ERR_PTR(-ENOMEM);
>
> cert->pub->paramlen = ctx->params_size;
> cert->pub->algo = ctx->key_algo;
> @@ -116,33 +114,23 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> /* Grab the signature bits */
> ret = x509_get_sig_params(cert);
> if (ret < 0)
> - goto error_decode;
> + return ERR_PTR(ret);
>
> /* Generate cert issuer + serial number key ID */
> kid = asymmetric_key_generate_id(cert->raw_serial,
> cert->raw_serial_size,
> cert->raw_issuer,
> cert->raw_issuer_size);
> - if (IS_ERR(kid)) {
> - ret = PTR_ERR(kid);
> - goto error_decode;
> - }
> + if (IS_ERR(kid))
> + return ERR_CAST(kid);
> cert->id = kid;
>
> /* Detect self-signed certificates */
> ret = x509_check_for_self_signed(cert);
> if (ret < 0)
> - goto error_decode;
> -
> - kfree(ctx);
> - return cert;
> + return ERR_PTR(ret);
>
> -error_decode:
> - kfree(ctx);
> -error_no_ctx:
> - x509_free_certificate(cert);
> -error_no_cert:
> - return ERR_PTR(ret);
> + return_ptr(cert);
> }
> EXPORT_SYMBOL_GPL(x509_cert_parse);
>
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index 97a886c..d2dfe50 100644
> --- a/crypto/asymmetric_keys/x509_parser.h
> +++ b/crypto/asymmetric_keys/x509_parser.h
> @@ -5,6 +5,7 @@
> * Written by David Howells ([email protected])
> */
>
> +#include <linux/cleanup.h>
> #include <linux/time.h>
> #include <crypto/public_key.h>
> #include <keys/asymmetric-type.h>
> @@ -44,6 +45,8 @@ struct x509_certificate {
> * x509_cert_parser.c
> */
> extern void x509_free_certificate(struct x509_certificate *cert);
> +DEFINE_FREE(x509_free_certificate, struct x509_certificate *,
> + if (!IS_ERR_OR_NULL(_T)) x509_free_certificate(_T))
> extern struct x509_certificate *x509_cert_parse(const void *data, size_t datalen);
> extern int x509_decode_time(time64_t *_t, size_t hdrlen,
> unsigned char tag,
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 6a4f00b..00ac715 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -161,12 +161,11 @@ int x509_check_for_self_signed(struct x509_certificate *cert)
> */
> static int x509_key_preparse(struct key_preparsed_payload *prep)
> {
> - struct asymmetric_key_ids *kids;
> - struct x509_certificate *cert;
> + struct x509_certificate *cert __free(x509_free_certificate);
> + struct asymmetric_key_ids *kids __free(kfree) = NULL;
> + char *p, *desc __free(kfree) = NULL;
> const char *q;
> size_t srlen, sulen;
> - char *desc = NULL, *p;
> - int ret;
>
> cert = x509_cert_parse(prep->data, prep->datalen);
> if (IS_ERR(cert))
> @@ -188,9 +187,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> }
>
> /* Don't permit addition of blacklisted keys */
> - ret = -EKEYREJECTED;
> if (cert->blacklisted)
> - goto error_free_cert;
> + return -EKEYREJECTED;
>
> /* Propose a description */
> sulen = strlen(cert->subject);
> @@ -202,10 +200,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> q = cert->raw_serial;
> }
>
> - ret = -ENOMEM;
> desc = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
> if (!desc)
> - goto error_free_cert;
> + return -ENOMEM;
> p = memcpy(desc, cert->subject, sulen);
> p += sulen;
> *p++ = ':';
> @@ -215,16 +212,14 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>
> kids = kmalloc(sizeof(struct asymmetric_key_ids), GFP_KERNEL);
> if (!kids)
> - goto error_free_desc;
> + return -ENOMEM;
> kids->id[0] = cert->id;
> kids->id[1] = cert->skid;
> kids->id[2] = asymmetric_key_generate_id(cert->raw_subject,
> cert->raw_subject_size,
> "", 0);
> - if (IS_ERR(kids->id[2])) {
> - ret = PTR_ERR(kids->id[2]);
> - goto error_free_kids;
> - }
> + if (IS_ERR(kids->id[2]))
> + return PTR_ERR(kids->id[2]);
>
> /* We're pinning the module by being linked against it */
> __module_get(public_key_subtype.owner);
> @@ -242,15 +237,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> cert->sig = NULL;
> desc = NULL;
> kids = NULL;
> - ret = 0;
> -
> -error_free_kids:
> - kfree(kids);
> -error_free_desc:
> - kfree(desc);
> -error_free_cert:
> - x509_free_certificate(cert);
> - return ret;
> + return 0;
> }
>
> static struct asymmetric_key_parser x509_key_parser = {
BR, Jarkko
On Sun, Jan 21, 2024 at 06:50:39PM +0100, Lukas Wunner wrote:
>
> * x509_cert_parse() now checks that "cert" is not an ERR_PTR() before
> calling x509_free_certificate() at end of scope. The compiler doesn't
> know that kzalloc() never returns an ERR_PTR().
How about moving the IS_ERR_OR_NULL check into x509_free_certificate
itself so that you can always call it?
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, Jan 26, 2024 at 01:37:28PM +0800, Herbert Xu wrote:
> On Sun, Jan 21, 2024 at 06:50:39PM +0100, Lukas Wunner wrote:
> >
> > * x509_cert_parse() now checks that "cert" is not an ERR_PTR() before
> > calling x509_free_certificate() at end of scope. The compiler doesn't
> > know that kzalloc() never returns an ERR_PTR().
>
> How about moving the IS_ERR_OR_NULL check into x509_free_certificate
> itself so that you can always call it?
But why? The cleanup.h insists on having an explicit check, so call will be
ignored completely on the branches when it's not needed. This is the pattern
that is currently being used. Why do we need a deviation here?
--
With Best Regards,
Andy Shevchenko
On Sun, Jan 28, 2024 at 03:32:31PM +0200, Andy Shevchenko wrote:
>
> But why? The cleanup.h insists on having an explicit check, so call will be
> ignored completely on the branches when it's not needed. This is the pattern
> that is currently being used. Why do we need a deviation here?
Well he was talking about eliminating duplicate checks, and one
way of doing that is by moving it into the cleanup function.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt