Re: [PATCH] sunrpc: Improve exception handling in krb5_etm_checksum()

[Chuck Lever]

On Sun, Dec 31, 2023 at 02:56:11PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  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