[PATCH 1/2] secmem: fix potential memory visibility issue

[Jussi Kivilinna]

* configure.ac (gcry_cv_have_sync_synchronize): New check.
* src/secmem.c (pooldesc_s): Make next pointer volatile.
(memory_barrier): New.
(_gcry_secmem_malloc_internal): Insert memory barrier between
pool->next and mainpool.next assigments.
(_gcry_private_is_secure): Update comments.
--

Signed-off-by: Jussi Kivilinna <jussi.kivilinna at iki.fi>
---
 0 files changed

diff --git a/configure.ac b/configure.ac
index cdce3394d..36906d51b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -858,6 +858,21 @@ if test "$gcry_cv_have_builtin_ctz" = "yes" ; then
 fi
 
 
+#
+# Check for __sync_synchronize intrinsic.
+#
+AC_CACHE_CHECK(for __sync_synchronize,
+       [gcry_cv_have_sync_synchronize],
+       [gcry_cv_have_sync_synchronize=no
+        AC_LINK_IFELSE([AC_LANG_PROGRAM([],
+          [__sync_synchronize(); return 0;])],
+          [gcry_cv_have_sync_synchronize=yes])])
+if test "$gcry_cv_have_sync_synchronize" = "yes" ; then
+   AC_DEFINE(HAVE_SYNC_SYNCHRONIZE, 1,
+             [Defined if compiler has '__sync_synchronize' intrinsic])
+fi
+
+
 #
 # Check for VLA support (variable length arrays).
 #
diff --git a/src/secmem.c b/src/secmem.c
index 3e091d88f..b6f07c54d 100644
--- a/src/secmem.c
+++ b/src/secmem.c
@@ -64,7 +64,7 @@ typedef struct pooldesc_s
 {
   /* A link to the next pool.  This is used to connect the overflow
    * pools.  */
-  struct pooldesc_s *next;
+  struct pooldesc_s * volatile next;
 
   /* A memory buffer used as allocation pool.  */
   void *mem;
@@ -118,6 +118,29 @@ GPGRT_LOCK_DEFINE (secmem_lock);
 #define ADDR_TO_BLOCK(addr) \
   (memblock_t *) (void *) ((char *) addr - BLOCK_HEAD_SIZE)
 
+
+/* Memory barrier */
+static inline void
+memory_barrier(void)
+{
+#ifdef HAVE_SYNC_SYNCHRONIZE
+#ifdef HAVE_GCC_ASM_VOLATILE_MEMORY
+  asm volatile ("":::"memory");
+#endif
+  /* Use GCC / clang intrinsic for memory barrier. */
+  __sync_synchronize();
+#else
+  /* Slow portable alternative, implement memory barrier by using mutex. */
+  gpgrt_lock_t tmp;
+  memset (&tmp, 0, sizeof(tmp));
+  gpgrt_lock_init (&tmp);
+  gpgrt_lock_lock (&tmp);
+  gpgrt_lock_unlock (&tmp);
+  gpgrt_lock_destroy (&tmp);
+#endif
+}
+
+
 /* Check whether P points into POOL.  */
 static inline int
 ptr_into_pool_p (pooldesc_t *pool, const void *p)
@@ -520,7 +543,7 @@ _gcry_secmem_get_flags (void)
 }
 
 
-/* This function initializes the main memory pool MAINPOOL.  Itis
+/* This function initializes the main memory pool MAINPOOL.  It is
  * expected to be called with the secmem lock held.  */
 static void
 _gcry_secmem_init_internal (size_t n)
@@ -670,8 +693,12 @@ _gcry_secmem_malloc_internal (size_t size, int xhint)
       pool->okay = 1;
 
       /* Take care: in _gcry_private_is_secure we do not lock and thus
-       * we assume that the second assignment below is atomic.  */
+       * we assume that the second assignment below is atomic.  Memory
+       * barrier prevents reordering of stores to new pool structure after
+       * MAINPOOL.NEXT assigment and prevents _gcry_private_is_secure seeing
+       * non-initialized POOL->NEXT pointers.  */
       pool->next = mainpool.next;
+      memory_barrier();
       mainpool.next = pool;
 
       /* After the first time we allocated an overflow pool, print a
@@ -811,9 +838,13 @@ _gcry_private_is_secure (const void *p)
 {
   pooldesc_t *pool;
 
-  /* We do no lock here because once a pool is allocatred it will not
-   * be removed anymore (except for gcry_secmem_term).  Further,
-   * adding a new pool to the list should be atomic.  */
+  /* We do no lock here because once a pool is allocated it will not
+   * be removed anymore (except for gcry_secmem_term).  Further, as
+   * assigment of POOL->NEXT in new pool structure is visible in
+   * this thread before assigment of MAINPOOL.NEXT, pool list can be
+   * iterated locklessly.  This visiblity is ensured by memory barrier
+   * between POOL->NEXT and MAINPOOL.NEXT assignments in
+   * _gcry_secmem_malloc_internal. */
   for (pool = &mainpool; pool; pool = pool->next)
     if (pool->okay && ptr_into_pool_p (pool, p))
       return 1;