From 10937d5867039afbf869c8514245ed7599b61307 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Wed, 20 Apr 2022 18:34:09 +0200 Subject: [PATCH] Refactor method construction pre- and post-condition The existing pre- and post-condition functions are supposed to check if methods have already been created and stored, using provider operation bits. This is supposed to only be done for "permanent" method stores. However, the way the pre-condition was called, it could not know if the set of implementations to be stored is likely to end up in a "permanent" or a temporary store. It needs access to the |no_store| flag returned by the provider's operation query function, because that call was done after the pre-condition was called. This requires a bit of refactoring, primarly of |algorithm_do_this()|, but also of |ossl_method_construct_precondition()|. Fixes #18150 Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/18151) --- crypto/core_algorithm.c | 103 +++++++++++++++++++++++++++------------- crypto/core_fetch.c | 24 ++++++++-- include/internal/core.h | 2 +- 3 files changed, 90 insertions(+), 39 deletions(-) diff --git a/crypto/core_algorithm.c b/crypto/core_algorithm.c index 5ff33eff7c747..6d1192f098d79 100644 --- a/crypto/core_algorithm.c +++ b/crypto/core_algorithm.c @@ -16,7 +16,8 @@ struct algorithm_data_st { OSSL_LIB_CTX *libctx; int operation_id; /* May be zero for finding them all */ - int (*pre)(OSSL_PROVIDER *, int operation_id, void *data, int *result); + int (*pre)(OSSL_PROVIDER *, int operation_id, int no_store, void *data, + int *result); void (*fn)(OSSL_PROVIDER *, const OSSL_ALGORITHM *, int no_store, void *data); int (*post)(OSSL_PROVIDER *, int operation_id, int no_store, void *data, @@ -24,10 +25,71 @@ struct algorithm_data_st { void *data; }; +/* + * Process one OSSL_ALGORITHM array, for the operation |cur_operation|, + * by constructing methods for all its implementations and adding those + * to the appropriate method store. + * Which method store is appropriate is given by |no_store| ("permanent" + * if 0, temporary if 1) and other data in |data->data|. + * + * Returns: + * -1 to quit adding algorithm implementations immediately + * 0 if not successful, but adding should continue + * 1 if successful so far, and adding should continue + */ +static int algorithm_do_map(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *map, + int cur_operation, int no_store, void *cbdata) +{ + struct algorithm_data_st *data = cbdata; + int ret = 0; + + /* Do we fulfill pre-conditions? */ + if (data->pre == NULL) { + /* If there is no pre-condition function, assume "yes" */ + ret = 1; + } else if (!data->pre(provider, cur_operation, no_store, data->data, + &ret)) { + /* Error, bail out! */ + return -1; + } + + /* + * If pre-condition not fulfilled don't add this set of implementations, + * but do continue with the next. This simply means that another thread + * got to it first. + */ + if (ret == 0) + return 1; + + if (map != NULL) { + const OSSL_ALGORITHM *thismap; + + for (thismap = map; thismap->algorithm_names != NULL; thismap++) + data->fn(provider, thismap, no_store, data->data); + } + + /* Do we fulfill post-conditions? */ + if (data->post == NULL) { + /* If there is no post-condition function, assume "yes" */ + ret = 1; + } else if (!data->post(provider, cur_operation, no_store, data->data, + &ret)) { + /* Error, bail out! */ + return -1; + } + + return ret; +} + +/* + * Given a provider, process one operation given by |data->operation_id|, or + * if that's zero, process all known operations. + * For each such operation, query the associated OSSL_ALGORITHM array from + * the provider, then process that array with |algorithm_do_map()|. + */ static int algorithm_do_this(OSSL_PROVIDER *provider, void *cbdata) { struct algorithm_data_st *data = cbdata; - int no_store = 0; /* Assume caching is ok */ int first_operation = 1; int last_operation = OSSL_OP__HIGHEST; int cur_operation; @@ -39,43 +101,18 @@ static int algorithm_do_this(OSSL_PROVIDER *provider, void *cbdata) for (cur_operation = first_operation; cur_operation <= last_operation; cur_operation++) { + int no_store = 0; /* Assume caching is ok */ const OSSL_ALGORITHM *map = NULL; int ret; - /* Do we fulfill pre-conditions? */ - if (data->pre == NULL) { - /* If there is no pre-condition function, assume "yes" */ - ret = 1; - } else { - if (!data->pre(provider, cur_operation, data->data, &ret)) - /* Error, bail out! */ - return 0; - } - - /* If pre-condition not fulfilled, go to the next operation */ - if (!ret) - continue; - map = ossl_provider_query_operation(provider, cur_operation, &no_store); - if (map != NULL) { - const OSSL_ALGORITHM *thismap; - - for (thismap = map; thismap->algorithm_names != NULL; thismap++) - data->fn(provider, thismap, no_store, data->data); - } + ret = algorithm_do_map(provider, map, cur_operation, no_store, data); ossl_provider_unquery_operation(provider, cur_operation, map); - /* Do we fulfill post-conditions? */ - if (data->post == NULL) { - /* If there is no post-condition function, assume "yes" */ - ret = 1; - } else { - if (!data->post(provider, cur_operation, no_store, data->data, - &ret)) - /* Error, bail out! */ - return 0; - } + if (ret < 0) + /* Hard error, bail out immediately! */ + return 0; /* If post-condition not fulfilled, set general failure */ if (!ret) @@ -88,7 +125,7 @@ static int algorithm_do_this(OSSL_PROVIDER *provider, void *cbdata) void ossl_algorithm_do_all(OSSL_LIB_CTX *libctx, int operation_id, OSSL_PROVIDER *provider, int (*pre)(OSSL_PROVIDER *, int operation_id, - void *data, int *result), + int no_store, void *data, int *result), void (*fn)(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *algo, int no_store, void *data), diff --git a/crypto/core_fetch.c b/crypto/core_fetch.c index 367f6ba8a47b1..6b25379f7bbce 100644 --- a/crypto/core_fetch.c +++ b/crypto/core_fetch.c @@ -24,16 +24,28 @@ struct construct_data_st { void *mcm_data; }; +static int is_temporary_method_store(int no_store, void *cbdata) +{ + struct construct_data_st *data = cbdata; + + return no_store && !data->force_store; +} + static int ossl_method_construct_precondition(OSSL_PROVIDER *provider, - int operation_id, void *cbdata, - int *result) + int operation_id, int no_store, + void *cbdata, int *result) { if (!ossl_assert(result != NULL)) { ERR_raise(ERR_LIB_CRYPTO, ERR_R_PASSED_NULL_PARAMETER); return 0; } - if (!ossl_provider_test_operation_bit(provider, operation_id, result)) + /* Assume that no bits are set */ + *result = 0; + + /* No flag bits for temporary stores */ + if (!is_temporary_method_store(no_store, cbdata) + && !ossl_provider_test_operation_bit(provider, operation_id, result)) return 0; /* @@ -56,7 +68,9 @@ static int ossl_method_construct_postcondition(OSSL_PROVIDER *provider, } *result = 1; - return no_store != 0 + + /* No flag bits for temporary stores */ + return is_temporary_method_store(no_store, cbdata) || ossl_provider_set_operation_bit(provider, operation_id); } @@ -82,7 +96,7 @@ static void ossl_method_construct_this(OSSL_PROVIDER *provider, * of the passed method. */ - if (data->force_store || !no_store) { + if (!is_temporary_method_store(no_store, data)) { /* If we haven't been told not to store, add to the global store */ data->mcm->put(NULL, method, provider, algo->algorithm_names, algo->property_definition, data->mcm_data); diff --git a/include/internal/core.h b/include/internal/core.h index 274e368aaaad3..545d985385d14 100644 --- a/include/internal/core.h +++ b/include/internal/core.h @@ -49,7 +49,7 @@ void *ossl_method_construct(OSSL_LIB_CTX *ctx, int operation_id, void ossl_algorithm_do_all(OSSL_LIB_CTX *libctx, int operation_id, OSSL_PROVIDER *provider, int (*pre)(OSSL_PROVIDER *, int operation_id, - void *data, int *result), + int no_store, void *data, int *result), void (*fn)(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *algo, int no_store, void *data),