From 566e908fa01eb91e0637347987bc61772d47aee1 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 | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/module/icp/io/skein_mod.c b/module/icp/io/skein_mod.c index 5ee36af12b..8992c5895e 100644 --- a/module/icp/io/skein_mod.c +++ b/module/icp/io/skein_mod.c @@ -494,7 +494,8 @@ skein_update(crypto_ctx_t *ctx, crypto_data_t *data, crypto_req_handle_t req) */ /*ARGSUSED*/ static int -skein_final(crypto_ctx_t *ctx, crypto_data_t *digest, crypto_req_handle_t req) +skein_final_nofree(crypto_ctx_t *ctx, crypto_data_t *digest, + crypto_req_handle_t req) { int error = CRYPTO_SUCCESS; @@ -525,6 +526,17 @@ skein_final(crypto_ctx_t *ctx, crypto_data_t *digest, crypto_req_handle_t req) else digest->cd_length = 0; + return (error); +} + +static int +skein_final(crypto_ctx_t *ctx, crypto_data_t *digest, crypto_req_handle_t req) +{ + int error = skein_final_nofree(ctx, digest, req); + + if (error == CRYPTO_BUFFER_TOO_SMALL) + return (error); + bzero(SKEIN_CTX(ctx), sizeof (*SKEIN_CTX(ctx))); kmem_free(SKEIN_CTX(ctx), sizeof (*(SKEIN_CTX(ctx)))); SKEIN_CTX_LVALUE(ctx) = NULL; @@ -560,7 +572,7 @@ skein_digest_atomic(crypto_provider_handle_t provider, if ((error = skein_update(&ctx, data, digest)) != CRYPTO_SUCCESS) goto out; - if ((error = skein_final(&ctx, data, digest)) != CRYPTO_SUCCESS) + if ((error = skein_final_nofree(&ctx, data, digest)) != CRYPTO_SUCCESS) goto out; out: @@ -669,7 +681,7 @@ skein_mac_atomic(crypto_provider_handle_t provider, if ((error = skein_update(&ctx, data, req)) != CRYPTO_SUCCESS) goto errout; - if ((error = skein_final(&ctx, mac, req)) != CRYPTO_SUCCESS) + if ((error = skein_final_nofree(&ctx, mac, req)) != CRYPTO_SUCCESS) goto errout; return (CRYPTO_SUCCESS);