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 <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #13954

1 files changed, 15 insertions, 3 deletions

diff --git a/module/icp/io/skein_mod.c b/module/icp/io/skein_mod.c
index 5ee36af12..8992c5895 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);