| Message ID | ace28d74f7c143fa28919214858a9ca90b6cf970.1712511262.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v4] X.509: Introduce scope-based x509_certificate allocation | expand |
On Sun, Apr 07, 2024 at 07:57:40PM +0200, Lukas Wunner wrote: > Add a DEFINE_FREE() clause for x509_certificate structs 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. > A third user will be introduced with the forthcoming SPDM library > (Security Protocol and Data Model) for PCI device authentication. > > Unlike most other DEFINE_FREE() clauses, this one checks for IS_ERR() > instead of NULL before calling x509_free_certificate() at end of scope. > That's because the "constructor" of x509_certificate structs, > x509_cert_parse(), returns a valid pointer or an ERR_PTR(), but never > NULL. > > Comparing the Assembler output before/after has shown they are identical, > save for the fact that gcc-12 always generates two return paths when > __cleanup() is used, one for the success case and one for the error case. > > In x509_cert_parse(), add a hint for the compiler that kzalloc() never > returns an ERR_PTR(). Otherwise the compiler adds a gratuitous IS_ERR() > check on return. Introduce an assume() macro for this which can be > re-used elsewhere in the kernel to provide hints for the compiler. > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Link: https://lore.kernel.org/all/20231003153937.000034ca@Huawei.com/ > Link: https://lwn.net/Articles/934679/ > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > Changes v3 -> v4: > Use passive mood in and drop the word "handy" from commit message (Jarkko). > > Link to v3: > https://lore.kernel.org/all/63cc7ab17a5064756e26e50bc605e3ff8914f05a.1708439875.git.lukas@wunner.de/ > > crypto/asymmetric_keys/x509_cert_parser.c | 43 ++++++++++++------------------- > crypto/asymmetric_keys/x509_parser.h | 3 +++ > crypto/asymmetric_keys/x509_public_key.c | 31 +++++++--------------- > include/linux/compiler.h | 2 ++ > 4 files changed, 30 insertions(+), 49 deletions(-) Patch applied. Thanks.
Apologies for late response, I've been sick as stated in some other LKML responses. On Sun Apr 7, 2024 at 8:57 PM EEST, Lukas Wunner wrote: > Add a DEFINE_FREE() clause for x509_certificate structs 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. > A third user will be introduced with the forthcoming SPDM library > (Security Protocol and Data Model) for PCI device authentication. > > Unlike most other DEFINE_FREE() clauses, this one checks for IS_ERR() > instead of NULL before calling x509_free_certificate() at end of scope. > That's because the "constructor" of x509_certificate structs, > x509_cert_parse(), returns a valid pointer or an ERR_PTR(), but never > NULL. +1 > Comparing the Assembler output before/after has shown they are identical, > save for the fact that gcc-12 always generates two return paths when > __cleanup() is used, one for the success case and one for the error case. > > In x509_cert_parse(), add a hint for the compiler that kzalloc() never > returns an ERR_PTR(). Otherwise the compiler adds a gratuitous IS_ERR() > check on return. Introduce an assume() macro for this which can be > re-used elsewhere in the kernel to provide hints for the compiler. > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Link: https://lore.kernel.org/all/20231003153937.000034ca@Huawei.com/ > Link: https://lwn.net/Articles/934679/ > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > Changes v3 -> v4: > Use passive mood in and drop the word "handy" from commit message (Jarkko). > > Link to v3: > https://lore.kernel.org/all/63cc7ab17a5064756e26e50bc605e3ff8914f05a.1708439875.git.lukas@wunner.de/ > > crypto/asymmetric_keys/x509_cert_parser.c | 43 ++++++++++++------------------- > crypto/asymmetric_keys/x509_parser.h | 3 +++ > crypto/asymmetric_keys/x509_public_key.c | 31 +++++++--------------- > include/linux/compiler.h | 2 ++ > 4 files changed, 30 insertions(+), 49 deletions(-) > > diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c > index 487204d..aeffbf6 100644 > --- a/crypto/asymmetric_keys/x509_cert_parser.c > +++ b/crypto/asymmetric_keys/x509_cert_parser.c > @@ -60,24 +60,24 @@ 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); > + assume(!IS_ERR(cert)); /* Avoid gratuitous IS_ERR() check on return */ > 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 +85,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 +95,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 +115,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..0688c22 100644 > --- a/crypto/asymmetric_keys/x509_parser.h > +++ b/crypto/asymmetric_keys/x509_parser.h > @@ -5,6 +5,7 @@ > * Written by David Howells (dhowells@redhat.com) > */ > > +#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(_T)) x509_free_certificate(_T)) +1 > 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 = { > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index c00cc6c..53666eb 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -148,6 +148,8 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > } while (0) > #endif > > +#define assume(cond) do { if (!(cond)) __builtin_unreachable(); } while (0) > + Should compiler.h additions be isolated to separate patches? > /* > * KENTRY - kernel entry point > * This can be used to annotate symbols (functions or data) that are used Other than that this looks good to me. BR, Jarkko
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c index 487204d..aeffbf6 100644 --- a/crypto/asymmetric_keys/x509_cert_parser.c +++ b/crypto/asymmetric_keys/x509_cert_parser.c @@ -60,24 +60,24 @@ 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); + assume(!IS_ERR(cert)); /* Avoid gratuitous IS_ERR() check on return */ 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 +85,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 +95,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 +115,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..0688c22 100644 --- a/crypto/asymmetric_keys/x509_parser.h +++ b/crypto/asymmetric_keys/x509_parser.h @@ -5,6 +5,7 @@ * Written by David Howells (dhowells@redhat.com) */ +#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(_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 = { diff --git a/include/linux/compiler.h b/include/linux/compiler.h index c00cc6c..53666eb 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -148,6 +148,8 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, } while (0) #endif +#define assume(cond) do { if (!(cond)) __builtin_unreachable(); } while (0) + /* * KENTRY - kernel entry point * This can be used to annotate symbols (functions or data) that are used
Add a DEFINE_FREE() clause for x509_certificate structs 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. A third user will be introduced with the forthcoming SPDM library (Security Protocol and Data Model) for PCI device authentication. Unlike most other DEFINE_FREE() clauses, this one checks for IS_ERR() instead of NULL before calling x509_free_certificate() at end of scope. That's because the "constructor" of x509_certificate structs, x509_cert_parse(), returns a valid pointer or an ERR_PTR(), but never NULL. Comparing the Assembler output before/after has shown they are identical, save for the fact that gcc-12 always generates two return paths when __cleanup() is used, one for the success case and one for the error case. In x509_cert_parse(), add a hint for the compiler that kzalloc() never returns an ERR_PTR(). Otherwise the compiler adds a gratuitous IS_ERR() check on return. Introduce an assume() macro for this which can be re-used elsewhere in the kernel to provide hints for the compiler. Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Link: https://lore.kernel.org/all/20231003153937.000034ca@Huawei.com/ Link: https://lwn.net/Articles/934679/ Signed-off-by: Lukas Wunner <lukas@wunner.de> --- Changes v3 -> v4: Use passive mood in and drop the word "handy" from commit message (Jarkko). Link to v3: https://lore.kernel.org/all/63cc7ab17a5064756e26e50bc605e3ff8914f05a.1708439875.git.lukas@wunner.de/ crypto/asymmetric_keys/x509_cert_parser.c | 43 ++++++++++++------------------- crypto/asymmetric_keys/x509_parser.h | 3 +++ crypto/asymmetric_keys/x509_public_key.c | 31 +++++++--------------- include/linux/compiler.h | 2 ++ 4 files changed, 30 insertions(+), 49 deletions(-)