| Message ID | 9561c78e-49a2-430c-a611-52806c0cdf25@web.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sunrpc: Improve exception handling in krb5_etm_checksum() | expand |
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.
… >> 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
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.
… > +++ 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
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; }