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@xxxxxxxxxx>
AuthorDate: Mon Jan 1 11:37:45 2024 -0500
Commit:     Chuck Lever <chuck.lever@xxxxxxxxxx>
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@xxxxxx>
    Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>

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