From: Lukas Wunner <lukas@wunner.de>
To: David Howells <dhowells@redhat.com>,
Herbert Xu <herbert@gondor.apana.org.au>,
"David S. Miller" <davem@davemloft.net>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: keyrings@vger.kernel.org, linux-crypto@vger.kernel.org,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Dan Williams <dan.j.williams@intel.com>,
Ard Biesheuvel <ardb@kernel.org>
Subject: [PATCH] X.509: Introduce scope-based x509_certificate allocation
Date: Sun, 21 Jan 2024 18:50:39 +0100 [thread overview]
Message-ID: <70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de> (raw)
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(-)
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 = {
--
2.40.1
next reply other threads:[~2024-01-21 17:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-21 17:50 Lukas Wunner [this message]
2024-01-22 19:29 ` [PATCH] X.509: Introduce scope-based x509_certificate allocation Jarkko Sakkinen
2024-02-12 11:35 ` Lukas Wunner
2024-01-26 5:37 ` Herbert Xu
2024-01-28 13:32 ` Andy Shevchenko
2024-01-29 2:32 ` Herbert Xu
2024-02-12 11:39 ` Lukas Wunner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de \
--to=lukas@wunner.de \
--cc=Jonathan.Cameron@huawei.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=ardb@kernel.org \
--cc=dan.j.williams@intel.com \
--cc=davem@davemloft.net \
--cc=dhowells@redhat.com \
--cc=herbert@gondor.apana.org.au \
--cc=keyrings@vger.kernel.org \
--cc=linux-crypto@vger.kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).