linux-kernel - Re: [PATCH] crypto: asymmetric_keys: fix uninitialized pointers with free attr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <33e351fb-4ecc-4894-b3c8-c0511d8fcc88@kernel.org>
Date: Sat, 22 Nov 2025 15:23:47 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Ignat Korchagin <ignat@...udflare.com>, Ally Heev <allyheev@...il.com>
Cc: David Howells <dhowells@...hat.com>, Lukas Wunner <lukas@...ner.de>,
 Herbert Xu <herbert@...dor.apana.org.au>,
 "David S. Miller" <davem@...emloft.net>, keyrings@...r.kernel.org,
 linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
 Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH] crypto: asymmetric_keys: fix uninitialized pointers with
 free attr

On 11/11/2025 14:12, Ignat Korchagin wrote:
> Hi,
> 
> On Wed, Nov 5, 2025 at 9:53 AM Ally Heev <allyheev@...il.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@...aro.org>
>> Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
>> Signed-off-by: Ally Heev <allyheev@...il.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