Archive-Team
/
zfs
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 103 Wiki Activity

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 <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #13954
This commit is contained in:
Richard Yao 2022-09-27 15:36:58 -04:00 committed by Tony Hutter
parent a2705b1dd5
commit 566e908fa0
1 changed files with 15 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
module/icp/io/skein_mod.c
View File

@ -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);