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 <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Link: https://lwn.net/Articles/934679/
Signed-off-by: Lukas Wunner <[email protected]>
---
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 ([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(_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
--
2.43.0
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 <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Link: https://lwn.net/Articles/934679/
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
> 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 ([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(_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