| Message ID | 70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | X.509: Introduce scope-based x509_certificate allocation | expand |
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/20231003153937.000034ca@Huawei.com/ 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 <Jonathan.Cameron@Huawei.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- 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 (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_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,
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?
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,
On Mon, Jan 22, 2024 at 09:29:33PM +0200, Jarkko Sakkinen wrote: > 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/20231003153937.000034ca@Huawei.com/ [...] > > 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? SPDM is a generic protocol for device authentication and measurement retrieval which has been adopted by the PCISIG and other consortiums. Jonathan's above-linked suggestion to add cleanup.h support for x509_certificate structs was in reply to a patch set I submitted to add SPDM and PCI device authentication support to the kernel. The cover letter of that patch set is available here: https://lore.kernel.org/all/cover.1695921656.git.lukas@wunner.de/ I am working on a v2 of that patch set which will take advantage of cleanup.h support for x509_certificate structs. So the present patch is a prerequisite for it. I don't think it's opportune or necessary to mention the patch set more verbosely in the commit message here, I just want to point out that the two functions converted by the patch will not be the only ones and that there's another use case coming up. I've just respun the patch and have amended the commit message with all the other feedback you gave. Thanks, Lukas
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 (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_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 = {
Jonathan suggests adding cleanup.h support for x509_certificate structs: https://lore.kernel.org/all/20231003153937.000034ca@Huawei.com/ 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 <Jonathan.Cameron@Huawei.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- 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(-)