From: Neil Horman <nhorman@openssl.org>
Date: Wed, 8 Oct 2025 12:20:36 +0000 (-0400)
Subject: Copy impls stack when calling ossl_method_store_do_all
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9ef4f426157e8a100b816c962fea1a168010bde2;p=thirdparty%2Fopenssl.git

Copy impls stack when calling ossl_method_store_do_all

PR https://github.com/openssl/openssl/pull/24782 introduced a copying of
the algs stack in ossl_method_store_do all, so that the subsequent
iteration of elements through alg_do_one could be done without a lock,
and without triggering a tsan error as reported in:
https://github.com/openssl/openssl/issues/24672

However, the problem wasn't completely fixed.  Issue:
https://github.com/openssl/openssl/issues/27726

Noted that, sometimes we still get a crash when iterating over each algs
impls stack.  This occurs because, even though we've cloned the algs to
a private data area, the impls stack for each alg still points to shared
data, which we are accessing without the benefit of a lock.  Because of
that, if some other thread calls a function that mutates the impl stack
(say ossl_method_store_remove()), we may encounter a NULL or garbage
value in one of the impl stack values, leading to an unexpected NULL
pointer or simmilar, which in turn leads to a crash.

Unfortunately we can't use a lock to create exclusive access here, as
there are several paths that lead to a recursive mutation of the stack,
which would deadlock.

So the only way that I see to prevent this (which is admittedly ugly),
is to not only clone the alg stack, but to duplicate each algs impl
stack with the read lock held, prior to doing the iteration.

Further, we've been unable to test this, as the problem is rare, and we
don't have a solid reproducer for the issue, but visual inspection
suggests this should fix that.  Hopefully:

Fixes #27726

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/28783)
---

diff --git a/crypto/property/property.c b/crypto/property/property.c
index a104e577401..aab184382f9 100644
--- a/crypto/property/property.c
+++ b/crypto/property/property.c
@@ -558,9 +558,21 @@ static void alg_copy(ossl_uintmax_t idx, ALGORITHM *alg, void *arg)
 {
     STACK_OF(ALGORITHM) *newalg = arg;
 
+    alg = OPENSSL_memdup(alg, sizeof(ALGORITHM));
+    if (alg == NULL)
+        return;
+
+    alg->impls = sk_IMPLEMENTATION_dup(alg->impls);
+
     (void)sk_ALGORITHM_push(newalg, alg);
 }
 
+static void del_tmpalg(ALGORITHM *alg)
+{
+    sk_IMPLEMENTATION_free(alg->impls);
+    OPENSSL_free(alg);
+}
+
 void ossl_method_store_do_all(OSSL_METHOD_STORE *store,
                               void (*fn)(int id, void *method, void *fnarg),
                               void *fnarg)
@@ -591,7 +603,7 @@ void ossl_method_store_do_all(OSSL_METHOD_STORE *store,
             for (j = 0; j < numimps; j++)
                 alg_do_one(alg, sk_IMPLEMENTATION_value(alg->impls, j), fn, fnarg);
         }
-        sk_ALGORITHM_free(tmpalgs);
+        sk_ALGORITHM_pop_free(tmpalgs, del_tmpalg);
     }
 }