From a2163a96ae8708bb083e8da7658c02a7047516ba Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Tue, 27 Sep 2022 15:36:58 -0400 Subject: [PATCH] Fix bad free in skein code Clang's static analyzer found a bad free caused by skein_mac_atomic(). It will allocate a context on the stack and then pass it to skein_final(), which attempts to free it. Upon inspection, skein_digest_atomic() also has the same problem. These functions were created to match the OpenSolaris ICP API, so I was curious how we avoided this in other providers and looked at the SHA2 code. It appears that SHA2 has a SHA2Final() helper function that is called by the exported sha2_mac_final()/sha2_digest_final() as well as the sha2_mac_atomic() and sha2_digest_atomic() functions. The real work is done in SHA2Final() while some checks and the free are done in sha2_mac_final()/sha2_digest_final(). We fix the use after free in the skein code by taking inspiration from the SHA2 code. We introduce a skein_final_nofree() that does most of the work, and make skein_final() into a function that calls it and then frees the memory. Reviewed-by: Brian Behlendorf Reviewed-by: Tony Hutter Signed-off-by: Richard Yao Closes #13954 --- module/icp/io/skein_mod.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/module/icp/io/skein_mod.c b/module/icp/io/skein_mod.c index a2ed6cedd8..221e1debd4 100644 --- a/module/icp/io/skein_mod.c +++ b/module/icp/io/skein_mod.c @@ -421,7 +421,7 @@ skein_update(crypto_ctx_t *ctx, crypto_data_t *data) * Supported output digest formats are raw, uio and mblk. */ static int -skein_final(crypto_ctx_t *ctx, crypto_data_t *digest) +skein_final_nofree(crypto_ctx_t *ctx, crypto_data_t *digest) { int error = CRYPTO_SUCCESS; @@ -452,6 +452,17 @@ skein_final(crypto_ctx_t *ctx, crypto_data_t *digest) else digest->cd_length = 0; + return (error); +} + +static int +skein_final(crypto_ctx_t *ctx, crypto_data_t *digest) +{ + int error = skein_final_nofree(ctx, digest); + + if (error == CRYPTO_BUFFER_TOO_SMALL) + return (error); + memset(SKEIN_CTX(ctx), 0, sizeof (*SKEIN_CTX(ctx))); kmem_free(SKEIN_CTX(ctx), sizeof (*(SKEIN_CTX(ctx)))); SKEIN_CTX_LVALUE(ctx) = NULL; @@ -485,7 +496,7 @@ skein_digest_atomic(crypto_mechanism_t *mechanism, crypto_data_t *data, if ((error = skein_update(&ctx, data)) != CRYPTO_SUCCESS) goto out; - if ((error = skein_final(&ctx, data)) != CRYPTO_SUCCESS) + if ((error = skein_final_nofree(&ctx, data)) != CRYPTO_SUCCESS) goto out; out: @@ -588,7 +599,7 @@ skein_mac_atomic(crypto_mechanism_t *mechanism, if ((error = skein_update(&ctx, data)) != CRYPTO_SUCCESS) goto errout; - if ((error = skein_final(&ctx, mac)) != CRYPTO_SUCCESS) + if ((error = skein_final_nofree(&ctx, mac)) != CRYPTO_SUCCESS) goto errout; return (CRYPTO_SUCCESS);