crypto/asymmetric_keys/x509_cert_parser.c | 11 +++++++---- crypto/asymmetric_keys/x509_public_key.c | 14 ++++++++------ 2 files changed, 15 insertions(+), 10 deletions(-)
Uninitialized pointers with `__free` attribute can cause undefined
behaviour as the memory assigned(randomly) to the pointer is freed
automatically when the pointer goes out of scope
crypto/asymmetric_keys doesn't have any bugs related to this as of now,
but, it is better to initialize and assign pointers with `__free` attr
in one statement to ensure proper scope-based cleanup
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
Signed-off-by: Ally Heev <allyheev@gmail.com>
---
crypto/asymmetric_keys/x509_cert_parser.c | 11 +++++++----
crypto/asymmetric_keys/x509_public_key.c | 14 ++++++++------
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 8df3fa60a44f80fbd71af17faeca2e92b6cc03ce..bfd2cb2a9d81e3c615dfd4fe6f41653869a8cbd6 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -60,12 +60,12 @@ EXPORT_SYMBOL_GPL(x509_free_certificate);
*/
struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
{
- struct x509_certificate *cert __free(x509_free_certificate);
- struct x509_parse_context *ctx __free(kfree) = NULL;
struct asymmetric_key_id *kid;
long ret;
- cert = kzalloc(sizeof(struct x509_certificate), GFP_KERNEL);
+ struct x509_certificate *cert __free(x509_free_certificate) = kzalloc(
+ sizeof(struct x509_certificate), GFP_KERNEL);
+
if (!cert)
return ERR_PTR(-ENOMEM);
cert->pub = kzalloc(sizeof(struct public_key), GFP_KERNEL);
@@ -74,7 +74,10 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
cert->sig = kzalloc(sizeof(struct public_key_signature), GFP_KERNEL);
if (!cert->sig)
return ERR_PTR(-ENOMEM);
- ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);
+
+ struct x509_parse_context *ctx __free(kfree) = kzalloc(
+ sizeof(struct x509_parse_context), GFP_KERNEL);
+
if (!ctx)
return ERR_PTR(-ENOMEM);
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 8409d7d36cb4f3582e15f9ee4d25f302b3b29358..818c9ab5d63940ff62c21666fd549d3a1ff07e67 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -148,13 +148,13 @@ int x509_check_for_self_signed(struct x509_certificate *cert)
*/
static int x509_key_preparse(struct key_preparsed_payload *prep)
{
- struct x509_certificate *cert __free(x509_free_certificate);
- struct asymmetric_key_ids *kids __free(kfree) = NULL;
- char *p, *desc __free(kfree) = NULL;
+ char *p;
const char *q;
size_t srlen, sulen;
- cert = x509_cert_parse(prep->data, prep->datalen);
+ struct x509_certificate *cert __free(x509_free_certificate) =
+ x509_cert_parse(prep->data, prep->datalen);
+
if (IS_ERR(cert))
return PTR_ERR(cert);
@@ -187,7 +187,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
q = cert->raw_serial;
}
- desc = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
+ char *desc __free(kfree) = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
if (!desc)
return -ENOMEM;
p = memcpy(desc, cert->subject, sulen);
@@ -197,7 +197,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
p = bin2hex(p, q, srlen);
*p = 0;
- kids = kmalloc(sizeof(struct asymmetric_key_ids), GFP_KERNEL);
+ struct asymmetric_key_ids *kids __free(kfree) = kmalloc(
+ sizeof(struct asymmetric_key_ids), GFP_KERNEL);
+
if (!kids)
return -ENOMEM;
kids->id[0] = cert->id;
---
base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
change-id: 20251105-aheev-uninitialized-free-attr-crypto-bc94ec1b2253
Best regards,
--
Ally Heev <allyheev@gmail.com>Hi,
On Wed, Nov 5, 2025 at 9:53 AM Ally Heev <allyheev@gmail.com> wrote:
>
> Uninitialized pointers with `__free` attribute can cause undefined
> behaviour as the memory assigned(randomly) to the pointer is freed
> automatically when the pointer goes out of scope
>
> crypto/asymmetric_keys doesn't have any bugs related to this as of now,
> but, it is better to initialize and assign pointers with `__free` attr
> in one statement to ensure proper scope-based cleanup
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> Signed-off-by: Ally Heev <allyheev@gmail.com>
> ---
> crypto/asymmetric_keys/x509_cert_parser.c | 11 +++++++----
> crypto/asymmetric_keys/x509_public_key.c | 14 ++++++++------
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 8df3fa60a44f80fbd71af17faeca2e92b6cc03ce..bfd2cb2a9d81e3c615dfd4fe6f41653869a8cbd6 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -60,12 +60,12 @@ EXPORT_SYMBOL_GPL(x509_free_certificate);
> */
> struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> {
> - struct x509_certificate *cert __free(x509_free_certificate);
Should this be just initialized to NULL instead of moving the declaration?
> - struct x509_parse_context *ctx __free(kfree) = NULL;
This pointer seems initialized. Is there still a problem?
> struct asymmetric_key_id *kid;
> long ret;
>
> - cert = kzalloc(sizeof(struct x509_certificate), GFP_KERNEL);
> + struct x509_certificate *cert __free(x509_free_certificate) = kzalloc(
> + sizeof(struct x509_certificate), GFP_KERNEL);
> +
> if (!cert)
> return ERR_PTR(-ENOMEM);
> cert->pub = kzalloc(sizeof(struct public_key), GFP_KERNEL);
> @@ -74,7 +74,10 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> cert->sig = kzalloc(sizeof(struct public_key_signature), GFP_KERNEL);
> if (!cert->sig)
> return ERR_PTR(-ENOMEM);
> - ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);
> +
> + struct x509_parse_context *ctx __free(kfree) = kzalloc(
> + sizeof(struct x509_parse_context), GFP_KERNEL);
> +
> if (!ctx)
> return ERR_PTR(-ENOMEM);
>
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 8409d7d36cb4f3582e15f9ee4d25f302b3b29358..818c9ab5d63940ff62c21666fd549d3a1ff07e67 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -148,13 +148,13 @@ int x509_check_for_self_signed(struct x509_certificate *cert)
> */
> static int x509_key_preparse(struct key_preparsed_payload *prep)
> {
> - struct x509_certificate *cert __free(x509_free_certificate);
And here: should we just initialize this to NULL?
> - struct asymmetric_key_ids *kids __free(kfree) = NULL;
> - char *p, *desc __free(kfree) = NULL;
Same here: these two pointers are initialized.
> + char *p;
> const char *q;
> size_t srlen, sulen;
>
> - cert = x509_cert_parse(prep->data, prep->datalen);
> + struct x509_certificate *cert __free(x509_free_certificate) =
> + x509_cert_parse(prep->data, prep->datalen);
> +
> if (IS_ERR(cert))
> return PTR_ERR(cert);
>
> @@ -187,7 +187,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> q = cert->raw_serial;
> }
>
> - desc = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
> + char *desc __free(kfree) = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
> if (!desc)
> return -ENOMEM;
> p = memcpy(desc, cert->subject, sulen);
> @@ -197,7 +197,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> p = bin2hex(p, q, srlen);
> *p = 0;
>
> - kids = kmalloc(sizeof(struct asymmetric_key_ids), GFP_KERNEL);
> + struct asymmetric_key_ids *kids __free(kfree) = kmalloc(
> + sizeof(struct asymmetric_key_ids), GFP_KERNEL);
> +
> if (!kids)
> return -ENOMEM;
> kids->id[0] = cert->id;
>
> ---
> base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> change-id: 20251105-aheev-uninitialized-free-attr-crypto-bc94ec1b2253
>
> Best regards,
> --
> Ally Heev <allyheev@gmail.com>
>
Ignat
On 11/11/2025 14:12, Ignat Korchagin wrote:
> Hi,
>
> On Wed, Nov 5, 2025 at 9:53 AM Ally Heev <allyheev@gmail.com> wrote:
>>
>> Uninitialized pointers with `__free` attribute can cause undefined
>> behaviour as the memory assigned(randomly) to the pointer is freed
>> automatically when the pointer goes out of scope
>>
>> crypto/asymmetric_keys doesn't have any bugs related to this as of now,
>> but, it is better to initialize and assign pointers with `__free` attr
>> in one statement to ensure proper scope-based cleanup
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
>> Signed-off-by: Ally Heev <allyheev@gmail.com>
>> ---
>> crypto/asymmetric_keys/x509_cert_parser.c | 11 +++++++----
>> crypto/asymmetric_keys/x509_public_key.c | 14 ++++++++------
>> 2 files changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
>> index 8df3fa60a44f80fbd71af17faeca2e92b6cc03ce..bfd2cb2a9d81e3c615dfd4fe6f41653869a8cbd6 100644
>> --- a/crypto/asymmetric_keys/x509_cert_parser.c
>> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
>> @@ -60,12 +60,12 @@ EXPORT_SYMBOL_GPL(x509_free_certificate);
>> */
>> struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
>> {
>> - struct x509_certificate *cert __free(x509_free_certificate);
>
> Should this be just initialized to NULL instead of moving the declaration?
No, it should not. That's not the syntax of cleanup.h... and if you do
not like that syntax (I fully understand), then please do not allow to
use cleanup.h in this/yours subsystem.
>
>> - struct x509_parse_context *ctx __free(kfree) = NULL;
>
Best regards,
Krzysztof
On Tue, Nov 11, 2025 at 6:42 PM Ignat Korchagin <ignat@cloudflare.com> wrote:
[...]
> Should this be just initialized to NULL instead of moving the declaration?
>
> > - struct x509_parse_context *ctx __free(kfree) = NULL;
>
> This pointer seems initialized. Is there still a problem?
>
> > struct asymmetric_key_id *kid;
> > long ret;
> >
> > - cert = kzalloc(sizeof(struct x509_certificate), GFP_KERNEL);
> > + struct x509_certificate *cert __free(x509_free_certificate) = kzalloc(
> > + sizeof(struct x509_certificate), GFP_KERNEL);
> > +
> > if (!cert)
> > return ERR_PTR(-ENOMEM);
> > cert->pub = kzalloc(sizeof(struct public_key), GFP_KERNEL);
> > @@ -74,7 +74,10 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> > cert->sig = kzalloc(sizeof(struct public_key_signature), GFP_KERNEL);
> > if (!cert->sig)
> > return ERR_PTR(-ENOMEM);
> > - ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);
> > +
> > + struct x509_parse_context *ctx __free(kfree) = kzalloc(
> > + sizeof(struct x509_parse_context), GFP_KERNEL);
> > +
> > if (!ctx)
> > return ERR_PTR(-ENOMEM);
> >
> > diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> > index 8409d7d36cb4f3582e15f9ee4d25f302b3b29358..818c9ab5d63940ff62c21666fd549d3a1ff07e67 100644
> > --- a/crypto/asymmetric_keys/x509_public_key.c
> > +++ b/crypto/asymmetric_keys/x509_public_key.c
> > @@ -148,13 +148,13 @@ int x509_check_for_self_signed(struct x509_certificate *cert)
> > */
> > static int x509_key_preparse(struct key_preparsed_payload *prep)
> > {
> > - struct x509_certificate *cert __free(x509_free_certificate);
>
> And here: should we just initialize this to NULL?
>
> > - struct asymmetric_key_ids *kids __free(kfree) = NULL;
> > - char *p, *desc __free(kfree) = NULL;
>
> Same here: these two pointers are initialized.
>
> > + char *p;
> > const char *q;
> > size_t srlen, sulen;
> >
> > - cert = x509_cert_parse(prep->data, prep->datalen);
> > + struct x509_certificate *cert __free(x509_free_certificate) =
> > + x509_cert_parse(prep->data, prep->datalen);
> > +
> > if (IS_ERR(cert))
> > return PTR_ERR(cert);
> >
> > @@ -187,7 +187,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> > q = cert->raw_serial;
> > }
> >
> > - desc = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
> > + char *desc __free(kfree) = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
> > if (!desc)
> > return -ENOMEM;
> > p = memcpy(desc, cert->subject, sulen);
> > @@ -197,7 +197,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> > p = bin2hex(p, q, srlen);
> > *p = 0;
> >
> > - kids = kmalloc(sizeof(struct asymmetric_key_ids), GFP_KERNEL);
> > + struct asymmetric_key_ids *kids __free(kfree) = kmalloc(
> > + sizeof(struct asymmetric_key_ids), GFP_KERNEL);
> > +
> > if (!kids)
> > return -ENOMEM;
> > kids->id[0] = cert->id;
> >
> > ---
> > base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> > change-id: 20251105-aheev-uninitialized-free-attr-crypto-bc94ec1b2253
> >
> > Best regards,
> > --
> > Ally Heev <allyheev@gmail.com>
> >
>
> Ignat
initializing to NULL at the declaration itself seems to be the right
solution here, as it conforms to the `variable declarations at the
top` rule too. I will send a new version with the changes
Regards,
Ally
© 2016 - 2025 Red Hat, Inc.