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

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

[Markus Elfring]

From: Markus Elfring <elfring@users.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@users.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

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@users.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@users.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

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

[Markus Elfring]

…
>> Thus use another label.
…
>> ---
>>  net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
…
> As has undoubtedly been pointed out in other forums, calling kfree()
> with a NULL argument is perfectly valid.

The function call “kfree(NULL)” is not really useful for error/exception handling
while it is tolerated at various source code places.


>                                          Since this small GFP_KERNEL
> allocation almost never fails, it's unlikely this change is going to
> make any difference except for readability.

I became curious if development interests can grow for the usage of
an additional label.
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources


> Now if we want to clean up the error flows in here to look more
> idiomatic, how about this:
…
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
…
> @@ -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);
…

I find it nice that you show another possible adjustment of corresponding identifiers.

Regards,
Markus

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

[Chuck Lever]

On Mon, Jan 01, 2024 at 12:24:59PM +0100, Markus Elfring wrote:
> …
> >> Thus use another label.
> …
> >> ---
> >>  net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> …
> > As has undoubtedly been pointed out in other forums, calling kfree()
> > with a NULL argument is perfectly valid.
> 
> The function call “kfree(NULL)” is not really useful for error/exception handling
> while it is tolerated at various source code places.
> 
> 
> >                                          Since this small GFP_KERNEL
> > allocation almost never fails, it's unlikely this change is going to
> > make any difference except for readability.
> 
> I became curious if development interests can grow for the usage of
> an additional label.
> https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources
> 
> 
> > Now if we want to clean up the error flows in here to look more
> > idiomatic, how about this:
> …
> > +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> …
> > @@ -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);
> …
> 
> I find it nice that you show another possible adjustment of corresponding identifiers.

I got curious, and tried using a static const buffer instead of
allocating one that always contains zeroes. The following compiles
and passes the SunRPC GSS Kunit tests:

commit 52d614882af007630072857b6b95a6d0fda23c1c
Author:     Chuck Lever <chuck.lever@oracle.com>
AuthorDate: Mon Jan 1 11:37:45 2024 -0500
Commit:     Chuck Lever <chuck.lever@oracle.com>
CommitDate: Mon Jan 1 11:47:06 2024 -0500

    SUNRPC: Use a static buffer for the checksum initialization vector
    
    Allocating and zeroing a buffer during every call to
    krb5_etm_checksum() is inefficient. Instead, set aside a static
    buffer that is the maximum crypto block size, and use a portion
    (or all) of that.
    
    Reported-by: Markus Elfring <Markus.Elfring@web.de>
    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index d2b02710ab07..82dc1eddf050 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -921,6 +921,8 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 offset, u32 len,
  * Caller provides the truncation length of the output token (h) in
  * cksumout.len.
  *
+ * Note that for RPCSEC, the "initial cipher state" is always all zeroes.
+ *
  * Return values:
  *   %GSS_S_COMPLETE: Digest computed, @cksumout filled in
  *   %GSS_S_FAILURE: Call failed
@@ -931,22 +933,19 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
                      int body_offset, struct xdr_netobj *cksumout)
 {
 	unsigned int ivsize = crypto_sync_skcipher_ivsize(cipher);
+	static const u8 iv[GSS_KRB5_MAX_BLOCKSIZE];
 	struct ahash_request *req;
 	struct scatterlist sg[1];
-	u8 *iv, *checksumdata;
+	u8 *checksumdata;
 	int err = -ENOMEM;
 
 	checksumdata = kmalloc(crypto_ahash_digestsize(tfm), GFP_KERNEL);
 	if (!checksumdata)
 		return GSS_S_FAILURE;
-	/* For RPCSEC, the "initial cipher state" is always all zeroes. */
-	iv = kzalloc(ivsize, GFP_KERNEL);
-	if (!iv)
-		goto out_free_mem;
 
 	req = ahash_request_alloc(tfm, GFP_KERNEL);
 	if (!req)
-		goto out_free_mem;
+		goto out_free_cksumdata;
 	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
 	err = crypto_ahash_init(req);
 	if (err)
@@ -970,8 +969,7 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
 
 out_free_ahash:
 	ahash_request_free(req);
-out_free_mem:
-	kfree(iv);
+out_free_cksumdata:
 	kfree_sensitive(checksumdata);
 	return err ? GSS_S_FAILURE : GSS_S_COMPLETE;
 }

I haven't tested this with an actual sec=krb5[ip] workload yet.



-- 
Chuck Lever

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

[Markus Elfring]

…
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
…
> @@ -970,8 +969,7 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
>
>  out_free_ahash:
>  	ahash_request_free(req);
> -out_free_mem:
> -	kfree(iv);
> +out_free_cksumdata:
>  	kfree_sensitive(checksumdata);
>  	return err ? GSS_S_FAILURE : GSS_S_COMPLETE;
>  }
…

How do you think about to use the identifier “out_free_checksumdata”?

Regards,
Markus