linux-kernel - Re: [PATCH] sunrpc: Improve exception handling in krb5_etm

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] [thread-next>] [day] [month] [year] [list]

Date: Sun, 31 Dec 2023 21:18:56 -0500
From: Chuck Lever <chuck.lever@...cle.com>
To: Markus Elfring <Markus.Elfring@....de>
Cc: linux-nfs@...r.kernel.org, netdev@...r.kernel.org,
        kernel-janitors@...r.kernel.org, Anna Schumaker <anna@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>, Dai Ngo <Dai.Ngo@...cle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Jakub Kicinski <kuba@...nel.org>, Jeff Layton <jlayton@...nel.org>,
        Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Paolo Abeni <pabeni@...hat.com>, Simo Sorce <simo@...hat.com>,
        Tom Talpey <tom@...pey.com>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sunrpc: Improve exception handling in krb5_etm_checksum()

On Sun, Dec 31, 2023 at 02:56:11PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Sun, 31 Dec 2023 14:43:05 +0100
> 
> The kfree() function was called in one case by
> the krb5_etm_checksum() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.
> 
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> index d2b02710ab07..5e2dc3eb8545 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -942,7 +942,7 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
>  	/* For RPCSEC, the "initial cipher state" is always all zeroes. */
>  	iv = kzalloc(ivsize, GFP_KERNEL);
>  	if (!iv)
> -		goto out_free_mem;
> +		goto out_free_checksum;
> 
>  	req = ahash_request_alloc(tfm, GFP_KERNEL);
>  	if (!req)
> @@ -972,6 +972,7 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
>  	ahash_request_free(req);
>  out_free_mem:
>  	kfree(iv);
> +out_free_checksum:
>  	kfree_sensitive(checksumdata);
>  	return err ? GSS_S_FAILURE : GSS_S_COMPLETE;
>  }
> --
> 2.43.0
> 

As has undoubtedly been pointed out in other forums, calling kfree()
with a NULL argument is perfectly valid. Since this small GFP_KERNEL
allocation almost never fails, it's unlikely this change is going to
make any difference except for readability.

Now if we want to clean up the error flows in here to look more
idiomatic, how about this:

diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index d2b02710ab07..6f3d3b3f7413 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -942,11 +942,11 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
 	/* For RPCSEC, the "initial cipher state" is always all zeroes. */
 	iv = kzalloc(ivsize, GFP_KERNEL);
 	if (!iv)
-		goto out_free_mem;
+		goto out_free_cksumdata;
 
 	req = ahash_request_alloc(tfm, GFP_KERNEL);
 	if (!req)
-		goto out_free_mem;
+		goto out_free_iv;
 	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
 	err = crypto_ahash_init(req);
 	if (err)
@@ -970,8 +970,9 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
 
 out_free_ahash:
 	ahash_request_free(req);
-out_free_mem:
+out_free_iv:
 	kfree(iv);
+out_free_cksumdata:
 	kfree_sensitive(checksumdata);
 	return err ? GSS_S_FAILURE : GSS_S_COMPLETE;
 }

Although, if we could guarantee the maximum size of the iv across
all encryption types, then a static constant array could be used
instead, I think.


-- 
Chuck Lever