Unverified Commit a2163a96 authored by Richard Yao's avatar Richard Yao Committed by GitHub
Browse files

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: default avatarBrian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: default avatarTony Hutter <hutter2@llnl.gov>
Signed-off-by: default avatarRichard Yao <richard.yao@alumni.stonybrook.edu>
Closes #13954 
parent f7bda2de
base NAS-116738 NAS-116738-prev NAS-117807 NAS-117845-dbg2-ozfs-mater NAS-117880 NAS-122915 NAS-122949 NAS-123279-2 NAS-124189-cobia NAS-124564 NAS-124699 NAS-124699-2 NAS-124699-3 NAS-124699-3-test NAS-125150 NAS-125882 NAS-125882-2 NAS-125916 NAS-125916-1 NAS-127352 NAS-127352-tn NAS-127702 NAS-127702-1 NAS-127702-df NAS-127822 NAS-127822-debug NAS-127888 NAS-127888-truenas TS-23.10 dragonfish/zfs-2.2.4-staging master nfsacl nfsacl-1 nfsacl-a nfsacl-review nfsv4acl nfsv4acls ozfs_master_test pkg-test raidz-expansion-rebase rel-v0.0.1 release/23.10-BETA.1 release/23.10-RC.1 release/23.10.0 release/23.10.1 release/23.10.1.2 release/23.10.1.3 release/23.10.2 release/24.04-BETA.1 release/24.04-RC.1 release/24.04.0 set-sast-config-1 stable/cobia stable/dragonfish test test-ci test_pkg testing-refine-branchout-process testing-refine-branchout-process2 tn_master truenas/NAS-127822-debug truenas/dragonfish-2.2.2-test truenas/zfs-2.2-release truenas/zfs-2.2.1-hutter truenas/zfs-2.2.1-hutter2 truenas/zfs-2.2.1-release truenas/zfs-2.2.1-release-dragonfish truenas/zfs-2.2.3-staging-dragonfish truenas/zfs-2.2.3-testing truenas/zfs-2.2.4-staging truenas/zfs-2.3-release truenas/zfs-2.3-testing truenas/zfs-cobia-rc1-test truenas/zvol-multi-taskq truenas/zvol-multiq truenas/zvol-multiq-clean truenas/zvol-thread-property zfetch_reorder zfetch_reorder10 zfs-json zfs-ozfs-master zvol-cleanup zvol-improvements-2.2.1 zvol-ro-property zvol-thread-property zvol_multi_taskq zvol_multiq TS-24.04-RC.1 TS-24.04-BETA.1 TS-23.10.2 TS-23.10.1.3 TS-23.10.1.2 TS-23.10.1.1 TS-23.10.1 TS-23.10.0.1 TS-23.10.0 TS-23.10-RC.1 TS-23.10-BETA.1 DN110M-CS-v2.0
No related merge requests found
Showing with 14 additions and 3 deletions
+14 -3
......@@ -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);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment