mlkem768: Fix missing jumps in error handling

Signed-off-by: Pavol Žáčik <pzacik@redhat.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>

src/mlkem768.c

@@ -79,6 +79,7 @@ static int mlkem768_keypair_gen(ssh_mlkem768_pubkey pubkey,
         SSH_LOG(SSH_LOG_WARNING,
                 "Failed to initialize ML-KEM-768 keygen: %s",
                 ERR_error_string(ERR_get_error(), NULL));
+        goto cleanup;
     }
 
     rc = EVP_PKEY_keygen(ctx, &pkey);
@@ -86,6 +87,7 @@ static int mlkem768_keypair_gen(ssh_mlkem768_pubkey pubkey,
         SSH_LOG(SSH_LOG_WARNING,
                 "Failed to perform ML-KEM-768 keygen: %s",
                 ERR_error_string(ERR_get_error(), NULL));
+        goto cleanup;
     }
 
     rc = EVP_PKEY_get_raw_public_key(pkey, pubkey, &pubkey_len);
@@ -93,6 +95,7 @@ static int mlkem768_keypair_gen(ssh_mlkem768_pubkey pubkey,
         SSH_LOG(SSH_LOG_WARNING,
                 "Failed to extract ML-KEM-768 public key: %s",
                 ERR_error_string(ERR_get_error(), NULL));
+        goto cleanup;
     }
 
     rc = EVP_PKEY_get_raw_private_key(pkey, privkey, &privkey_len);
@@ -100,6 +103,7 @@ static int mlkem768_keypair_gen(ssh_mlkem768_pubkey pubkey,
         SSH_LOG(SSH_LOG_WARNING,
                 "Failed to extract ML-KEM-768 private key: %s",
                 ERR_error_string(ERR_get_error(), NULL));
+        goto cleanup;
     }
 
     ret = SSH_OK;
@@ -184,6 +188,7 @@ static int mlkem768_decapsulate(const ssh_mlkem768_privkey privkey,
         SSH_LOG(SSH_LOG_WARNING,
                 "Failed to create ML-KEM-768 context: %s",
                 ERR_error_string(ERR_get_error(), NULL));
+        goto cleanup;
     }
 
     ctx = EVP_PKEY_CTX_new_from_pkey(NULL, pkey, NULL);

tests/CMakeLists.txt

@@ -389,10 +389,10 @@ if (FUZZ_TESTING)
 endif()
 
 add_custom_target(test_memcheck
-    # FIXME: The threads_pki_rsa test is skipped under valgrind as it times out
+    # FIXME: The pkd_hello_i1 test is skipped under valgrind as it times out
     # Passing suppression file is also stupid so lets go with override here:
     # https://stackoverflow.com/a/56116311
-    COMMAND ${CMAKE_CTEST_COMMAND} -E torture_threads_pki_rsa -E pkd_hello_i1
+    COMMAND ${CMAKE_CTEST_COMMAND} -E pkd_hello_i1
         --output-on-failure --force-new-ctest-process --test-action memcheck
         --overwrite MemoryCheckSuppressionFile=${CMAKE_SOURCE_DIR}/tests/valgrind.supp
     WORKING_DIRECTORY "${CMAKE_BINARY_DIR}")

tests/unittests/torture_threads_pki_rsa.c

@@ -58,14 +58,8 @@ static int run_on_threads(void *(*func)(void *))
     }
 
     for (i = 0; i < NUM_THREADS; ++i) {
-        void *p = NULL;
+        rc = pthread_join(threads[i], NULL);
-        uint64_t *result;
-
-        rc = pthread_join(threads[i], &p);
         assert_int_equal(rc, 0);
-
-        result = (uint64_t *)p;
-        assert_null(result);
     }
 
     return rc;
@@ -164,7 +158,7 @@ static void *thread_pki_rsa_import_pubkey_file(void *threadid)
 
     SSH_KEY_FREE(pubkey);
 
-    pthread_exit(NULL);
+    return NULL;
 }
 
 static void torture_pki_rsa_import_pubkey_file(void **state)
@@ -197,8 +191,7 @@ static void *thread_pki_rsa_import_privkey_base64_NULL_key(void *threadid)
                                        NULL,
                                        NULL);
     assert_true(rc == -1);
-
+    return NULL;
-    pthread_exit(NULL);
 }
 
 static void torture_pki_rsa_import_privkey_base64_NULL_key(void **state){
@@ -225,7 +218,8 @@ static void *thread_pki_rsa_import_privkey_base64_NULL_str(void *threadid)
     assert_true(rc == -1);
 
     SSH_KEY_FREE(key);
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_import_privkey_base64_NULL_str(void **state){
@@ -267,7 +261,7 @@ static void *thread_pki_rsa_import_privkey_base64(void *threadid)
     free(key_str);
     SSH_KEY_FREE(key);
 
-    pthread_exit(NULL);
+    return NULL;
 }
 
 static void torture_pki_rsa_import_privkey_base64(void **state)
@@ -310,7 +304,8 @@ static void *thread_pki_rsa_publickey_from_privatekey(void *threadid)
 
     SSH_KEY_FREE(key);
     SSH_KEY_FREE(pubkey);
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_publickey_from_privatekey(void **state)
@@ -383,7 +378,8 @@ static void *thread_pki_rsa_copy_cert_to_privkey(void *threadid)
     SSH_KEY_FREE(cert);
     SSH_KEY_FREE(privkey);
     SSH_KEY_FREE(pubkey);
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_copy_cert_to_privkey(void **state)
@@ -416,7 +412,8 @@ static void *thread_pki_rsa_import_cert_file(void *threadid)
     assert_true(rc == 1);
 
     SSH_KEY_FREE(cert);
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_import_cert_file(void **state)
@@ -467,7 +464,8 @@ static void *thread_pki_rsa_publickey_base64(void *threadid)
     free(b64_key);
     free(key_buf);
     SSH_KEY_FREE(key);
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_publickey_base64(void **state)
@@ -531,7 +529,8 @@ static void *thread_pki_rsa_duplicate_key(void *threadid)
     SSH_KEY_FREE(privkey_dup);
     SSH_STRING_FREE_CHAR(b64_key);
     SSH_STRING_FREE_CHAR(b64_key_gen);
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_duplicate_key(void **state)
@@ -614,7 +613,8 @@ static void *thread_pki_rsa_generate_key(void *threadid)
     SSH_KEY_FREE(pubkey);
 
     ssh_free(session);
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_generate_key(void **state)
@@ -672,7 +672,8 @@ static void *thread_pki_rsa_import_privkey_base64_passphrase(void *threadid)
     assert_true(rc == -1);
     SSH_KEY_FREE(key);
 #endif
-    pthread_exit(NULL);
+
+    return NULL;
 }
 
 static void torture_pki_rsa_import_privkey_base64_passphrase(void **state)
@@ -723,14 +724,8 @@ static void torture_mixed(void **state)
 
     for (f = 0; f < NUM_TESTS; f++) {
         for (i = 0; i < NUM_THREADS; ++i) {
-            void *p = NULL;
+            rc = pthread_join(threads[f][i], NULL);
-            uint64_t *result = NULL;
-
-            rc = pthread_join(threads[f][i], &p);
             assert_int_equal(rc, 0);
-
-            result = (uint64_t *)p;
-            assert_null(result);
         }
     }
 }

tests/valgrind.supp

@@ -140,6 +140,40 @@
    fun:FIPS_mode_set
    fun:OPENSSL_init_library
 }
+{
+   Threads + Failed PEM decoder do not play well openssl/openssl#29077
+   Memcheck:Leak
+   match-leak-kinds: definite
+   fun:malloc
+   fun:CRYPTO_malloc
+   fun:CRYPTO_zalloc
+   fun:ossl_rcu_read_lock
+   fun:module_find
+   fun:module_run
+   fun:CONF_modules_load
+   fun:CONF_modules_load_file_ex
+   fun:ossl_config_int
+   fun:ossl_config_int
+   fun:ossl_init_config
+   fun:ossl_init_config_ossl_
+   fun:__pthread_once_slow.isra.0
+   fun:pthread_once@@GLIBC_2.34
+   fun:CRYPTO_THREAD_run_once
+   fun:OPENSSL_init_crypto
+   fun:ossl_provider_doall_activated
+   fun:ossl_algorithm_do_all
+   fun:ossl_method_construct.constprop.0
+   fun:inner_evp_generic_fetch.constprop.0
+   fun:evp_generic_do_all
+   fun:EVP_KEYMGMT_do_all_provided
+   fun:ossl_decoder_ctx_setup_for_pkey
+   fun:OSSL_DECODER_CTX_new_for_pkey
+   fun:pem_read_bio_key_decoder
+   fun:pem_read_bio_key
+   fun:PEM_read_bio_PrivateKey_ex
+   fun:pki_private_key_from_base64
+   ...
+}
 # Cmocka
 {
    This looks like leak from cmocka when the forked server is not properly terminated
@@ -290,13 +324,11 @@
    fun:malloc
    ...
    fun:krb5_gss_save_error_string
-   fun:UnknownInlinedFun
+   ...
    fun:acquire_cred_context.isra.0
    fun:acquire_cred_from.isra.0
    fun:gss_add_cred_from
    fun:gss_acquire_cred_from
-   ...
-   fun:gss_acquire_cred
 }
 {
    error string from gss init sec context
@@ -305,7 +337,7 @@
    fun:malloc
    ...
    fun:krb5_gss_save_error_string
-   fun:UnknownInlinedFun
+   ...
    fun:krb5_gss_init_sec_context_ext
    fun:krb5_gss_init_sec_context
    fun:gss_init_sec_context