[PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[Lukas Wunner]

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.

I've compared the Assembler output before/after and 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 a handy 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 v2 -> v3:
 * Add curly braces to "do { } while" loop in assume() macro (Andy).
 * Rephrase commit message: Drop first paragraph, use "Link:" tag for
   LWN article (Jarkko).

Link to v2:
 https://lore.kernel.org/all/4143b15418c4ecf87ddeceb36813943c3ede17aa.1707734526.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))
 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 bb1339c..aee74ba 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -139,6 +139,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

Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[Jarkko Sakkinen]

On Tue Feb 20, 2024 at 3:10 PM UTC, 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.

I think you are adding scope-based memory management and not
DEFINE_FREE(). Otherwise, this would be one-liner patch.

I'm not sure if the last sentence adds more than clutter as this
patch has nothing to do with SPDM changes per se.

> 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.
>
> I've compared the Assembler output before/after and 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.

Use passive as commit message is not a personal letter.

>
> 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 a handy assume() macro for this which can be
> re-used elsewhere in the kernel to provide hints for the compiler.

Does not explain why it is "handy".

I don't see a story here but instead I see bunch of disordered tecnical
terms.

We have the code diff for detailed technical stuff. The commit message
should simply explain why we want this and what it does for us. And we
zero care about PCI changes in the scope of this patch, especially since
this is not part of such patch set.

BR, Jarkko

Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[Jarkko Sakkinen]

On Tue Feb 20, 2024 at 6:00 PM UTC, Jarkko Sakkinen wrote:
> On Tue Feb 20, 2024 at 3:10 PM UTC, 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.
>
> I think you are adding scope-based memory management and not
> DEFINE_FREE(). Otherwise, this would be one-liner patch.
>
> I'm not sure if the last sentence adds more than clutter as this
> patch has nothing to do with SPDM changes per se.
>
> > 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.
> >
> > I've compared the Assembler output before/after and 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.
>
> Use passive as commit message is not a personal letter.
>
> >
> > 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 a handy assume() macro for this which can be
> > re-used elsewhere in the kernel to provide hints for the compiler.
>
> Does not explain why it is "handy".
>
> I don't see a story here but instead I see bunch of disordered tecnical
> terms.
>
> We have the code diff for detailed technical stuff. The commit message
> should simply explain why we want this and what it does for us. And we
> zero care about PCI changes in the scope of this patch, especially since
> this is not part of such patch set.

I mean think it this way.

What is the most important function of a commit message? Well, it comes
when the commit is in the mainline. It reminds of the *reasons* why a
change was made and this commit message does not really serve well in
that role.

BR, Jarkko

Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[Herbert Xu]

On Tue, Feb 20, 2024 at 04:10:39PM +0100, Lukas Wunner wrote:
>
> 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 a handy assume() macro for this which can be
> re-used elsewhere in the kernel to provide hints for the compiler.

Would it be possible to move the use of assume into the kzalloc
declaration instead? Perhaps by turning it into a static inline
wrapper that does the "assume"?

Otherwise as time goes on we'll have a proliferation of these
"assume"s all over the place.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[Lukas Wunner]

On Fri, Mar 01, 2024 at 06:10:33PM +0800, Herbert Xu wrote:
> On Tue, Feb 20, 2024 at 04:10:39PM +0100, Lukas Wunner wrote:
> >
> > 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 a handy assume() macro for this which can be
> > re-used elsewhere in the kernel to provide hints for the compiler.
> 
> Would it be possible to move the use of assume into the kzalloc
> declaration instead? Perhaps by turning it into a static inline
> wrapper that does the "assume"?
> 
> Otherwise as time goes on we'll have a proliferation of these
> "assume"s all over the place.

I've tried moving the assume(!IS_ERR()) to kmalloc() (which already is
a static inline), but that increased total vmlinux size by 448 bytes.
I was expecting pushback due to the size increase, hence kept the
assume() local to x509_cert_parse().

There's a coccinelle rule which warns if an IS_ERR() check is performed
on a kmalloc'ed pointer (scripts/coccinelle/null/eno.cocci), hence there
don't seem to be any offenders left in the tree which use this antipattern
and adding assume(!IS_ERR()) to kmalloc() doesn't have any positive effect
beyond avoiding the single unnecessary check in x509_cert_parse().

If you don't like the assume(!IS_ERR(cert)) in x509_cert_parse(),
I can respin the patch to drop it.  The unnecessary check which it
avoids only occurs in the error path.  If the certificate can be
parsed without error, there's no unnecessary check.  It still
triggered my OCD when scrutinizing the disassembled code and
sufficiently annoyed me that I wanted to get rid of it,
but in reality it's not such a big deal.

Thanks,

Lukas

Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[Herbert Xu]

On Sat, Mar 02, 2024 at 09:27:51AM +0100, Lukas Wunner wrote:
>
> I've tried moving the assume(!IS_ERR()) to kmalloc() (which already is
> a static inline), but that increased total vmlinux size by 448 bytes.
> I was expecting pushback due to the size increase, hence kept the
> assume() local to x509_cert_parse().

OK if you've already tried it then I'll take this as it stands.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation

[Lukas Wunner]

On Tue, Feb 20, 2024 at 06:00:41PM +0000, Jarkko Sakkinen wrote:
> On Tue Feb 20, 2024 at 3:10 PM UTC, 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.
> 
> I think you are adding scope-based memory management and not
> DEFINE_FREE(). Otherwise, this would be one-liner patch.

Above it states very clearly: "and use it in x509_cert_parse() and
x509_key_preparse()".

That's the reason it is not a one-liner patch.


> I'm not sure if the last sentence adds more than clutter as this
> patch has nothing to do with SPDM changes per se.

I disagree.  It is important as a justification for the change that
the two functions converted here are not going to be the only users,
but that there's a third one coming up.


> > I've compared the Assembler output before/after and 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.
> 
> Use passive as commit message is not a personal letter.

Okay, I will respin and change to passive mood.


> I don't see a story here but instead I see bunch of disordered tecnical
> terms.

That doesn't sound like constructive criticism.


> We have the code diff for detailed technical stuff. The commit message
> should simply explain why we want this and what it does for us.
[...]
> What is the most important function of a commit message? Well, it comes
> when the commit is in the mainline. It reminds of the *reasons* why a
> change was made and this commit message does not really serve well in
> that role.

The reason for the commit is that Jonathan requested it during code
review of my PCI device authentication patches.  I've been stating
this very clearly in the first iteration of the present patch.
You asked that I delete the sentence and instead use a Suggested-by.

Perhaps it would have been better had I not listened to you.
Because now you seem to have forgotten the reason for the patch,
which, again, you asked me to delete.

FWIW, here's the link to Jonathan's review:
https://lore.kernel.org/all/20231003153937.000034ca@Huawei.com/

And here's the quote with his explicit request to use cleanup.h:

   "Maybe cleanup.h magic?  Seems like it would simplify error paths here a
    tiny bit. Various other cases follow, but I won't mention this every time
    [...]
    Even this could be done with the cleanup.h stuff with appropriate
    pointer stealing and hence allow direct returns.
    This is the sort of case that I think really justifies that stuff."

Thanks,

Lukas