linux-kernel - Re: [PATCH] crypto: qce - revert "use __free() for a buffer that's always freed"

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: <CAMRc=Md19mjs3J+RKTGg--bC+G6yFk5J00wCMohqtt2_O0k=9Q@mail.gmail.com>
Date: Thu, 19 Dec 2024 09:03:15 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Thara Gopinath <thara.gopinath@...il.com>, Herbert Xu <herbert@...dor.apana.org.au>, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	linux-crypto@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev, patches@...ts.linux.dev, 
	Linux Kernel Functional Testing <lkft@...aro.org>
Subject: Re: [PATCH] crypto: qce - revert "use __free() for a buffer that's
 always freed"

On Wed, Dec 18, 2024 at 9:13 PM Nathan Chancellor <nathan@...nel.org> wrote:
>
> Commit ce8fd0500b74 ("crypto: qce - use __free() for a buffer that's
> always freed") introduced a buggy use of __free(), which clang
> rightfully points out:
>
>   drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto statement to its label
>     365 |                 goto err_free_ahash;
>         |                 ^
>   drivers/crypto/qce/sha.c:373:6: note: jump bypasses initialization of variable with __attribute__((cleanup))
>     373 |         u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
>         |             ^
>
> Jumping over a variable declared with the cleanup attribute does not
> prevent the cleanup function from running; instead, the cleanup function
> is called with an uninitialized value.
>
> Moving the declaration back to the top function with __free() and a NULL
> initialization would resolve the bug but that is really not much
> different from the original code. Since the function is so simple and
> there is no functional reason to use __free() here, just revert the
> original change to resolve the issue.
>
> Fixes: ce8fd0500b74 ("crypto: qce - use __free() for a buffer that's always freed")
> Reported-by: Linux Kernel Functional Testing <lkft@...aro.org>
> Closes: https://lore.kernel.org/CA+G9fYtpAwXa5mUQ5O7vDLK2xN4t-kJoxgUe1ZFRT=AGqmLSRA@mail.gmail.com/
> Signed-off-by: Nathan Chancellor <nathan@...nel.org>
> ---

Yeah, that was probably overkill but I thought it makes sense if I'm
reworking the driver anyway.

Feel free to kill it.

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>