]> git.ipfire.org Git - thirdparty/elfutils.git/commitdiff
libdw: Fix eu_search_tree TOCTOU bugs
authorAaron Merey <amerey@redhat.com>
Mon, 2 Jun 2025 21:06:31 +0000 (17:06 -0400)
committerAaron Merey <amerey@redhat.com>
Mon, 2 Jun 2025 21:06:31 +0000 (17:06 -0400)
eu_tfind is used to facilitate lazy loading throughout libdw.
If a result is not found via eu_tfind, work is done to load
the result and cache it in an eu_search_tree.

Some calls to eu_tfind allow for TOCTOU bugs.  Multiple threads
might race to call eu_tfind on some result that hasn't yet been
cached.  Multiple threads may then attempt to load the result
which might cause an unnecessary amount of memory to be allocated.
Additionally this memory may not get released when the associated
libdw data structure is freed.

Fix this by adding additional locking to ensure that only one
thread performs lazy loading.

One approach used in this patch is to preserve calls to eu_tfind
without additional locking, but when the result isn't found then
a lock is then used to synchronize access to the lazy loading code.
An extra eu_tfind call has been added at the start of these critical
section to synchronize verification that lazy loading should proceed.

Another approach used is to simply synchronize entire calls to
functions where lazy loading via eu_tfind might occur (__libdw_find_fde
and __libdw_intern_expression).  In this case, new _nolock variants of
the eu_t* functions are used to avoid unnecessary double locking.

lib/
* eu-search.c: Add eu_tsearch_nolock, eu_tfind_nolock and
eu_tdelete_nolock functions.
* eu-search.h: Ditto.

libdw/
* cfi.h (struct Dwarf_CFI_s): Declare new mutex.
* dwarf_begin_elf.c (valid_p): Initialize all locks for fake CUs.
* dwarf_cfi_addrframe.c (dwarf_cfi_addrframe): Place lock around
__libdw_find_fde.
* dwarf_end.c (cu_free): Deallocate all locks unconditionally,
whether or not the CU is fake.
* dwarf_frame_cfa.c (dwarf_frame_cfa): Place lock around
__libdw_intern_expression.
* dwarf_frame_register.c (dwarf_frame_register): Ditto.
* dwarf_getcfi.c (dwarf_getcfi): Initialize cfi lock.
* dwarf_getlocation.c (is_constant_offset): Synchronize access
to lazy loading section.
(getlocation): Place lock around __libdw_intern_expression.
* dwarf_getmacros.c (cache_op_table): Synchronize access to lazy
loading section.
* frame-cache.c (__libdw_destroy_frame_cache): Free Dwarf_CFI
mutex.
* libdwP.h (struct Dwarf): Update macro_lock comment.
(struct Dwarf_CU): Declare new mutex.
libdw_findcu.c (__libdw_intern_next_unit): Initialize
intern_lock.
(__libdw_findcu): Adjust locking so that the first eu_tfind
can be done without extra lock overhead.

Signed-off-by: Aaron Merey <amerey@redhat.com>
14 files changed:
lib/eu-search.h
libdw/cfi.h
libdw/dwarf_begin_elf.c
libdw/dwarf_cfi_addrframe.c
libdw/dwarf_end.c
libdw/dwarf_frame_cfa.c
libdw/dwarf_frame_register.c
libdw/dwarf_getcfi.c
libdw/dwarf_getlocation.c
libdw/dwarf_getmacros.c
libdw/fde.c
libdw/frame-cache.c
libdw/libdwP.h
libdw/libdw_findcu.c

index 67b54c18667bf914d2ed78f9b020e04b6c6540f9..4299e11564db1103412e5d7adca0854ef1d09651 100644 (file)
@@ -52,6 +52,30 @@ extern void *eu_tfind (const void *key, search_tree *tree,
 extern void *eu_tdelete (const void *key, search_tree *tree,
                         int (*compare)(const void *, const void *));
 
+/* Search TREE for KEY and add KEY if not found.  No locking is performed.  */
+static inline void *
+eu_tsearch_nolock (const void *key, search_tree *tree,
+                  int (*compare)(const void *, const void *))
+{
+  return tsearch (key, &tree->root, compare);
+}
+
+/* Search TREE for KEY.  No locking is performed.  */
+static inline void *
+eu_tfind_nolock (const void *key, search_tree *tree,
+                int (*compare)(const void *, const void *))
+{
+  return tfind (key, &tree->root, compare);
+}
+
+/* Delete key from TREE.  No locking is performed.  */
+static inline void *
+eu_tdelete_nolock (const void *key, search_tree *tree,
+                  int (*compare)(const void *, const void *))
+{
+  return tdelete (key, &tree->root, compare);
+}
+
 /* Free all nodes from TREE.  */
 void eu_tdestroy (search_tree *tree, void (*free_node)(void *));
 
index d0134243fd1b0306c0ce0cb31cc78393ee6c3b29..f0296de7371d81b756bbf8aef763b252be863f76 100644 (file)
@@ -98,6 +98,10 @@ struct Dwarf_CFI_s
   /* Search tree for parsed DWARF expressions, indexed by raw pointer.  */
   search_tree expr_tree;
 
+  /* Should be held when calling __libdw_find_fde, __libdw_fde_by_offset and
+     when __libdw_intern_expression is called with Dwarf_CFI members.  */
+  mutex_define(, lock);
+
   /* Backend hook.  */
   struct ebl *ebl;
 
index 58a309e9e7465a261b090525630a3ce7438e67f3..63e2805d3439627c2f60d7229384eb761c5b806d 100644 (file)
@@ -359,6 +359,11 @@ valid_p (Dwarf *result)
          result->fake_loc_cu->version = 4;
          result->fake_loc_cu->split = NULL;
          eu_search_tree_init (&result->fake_loc_cu->locs_tree);
+         rwlock_init (result->fake_loc_cu->abbrev_lock);
+         rwlock_init (result->fake_loc_cu->split_lock);
+         mutex_init (result->fake_loc_cu->src_lock);
+         mutex_init (result->fake_loc_cu->str_off_base_lock);
+         mutex_init (result->fake_loc_cu->intern_lock);
        }
     }
 
@@ -387,6 +392,11 @@ valid_p (Dwarf *result)
          result->fake_loclists_cu->version = 5;
          result->fake_loclists_cu->split = NULL;
          eu_search_tree_init (&result->fake_loclists_cu->locs_tree);
+         rwlock_init (result->fake_loclists_cu->abbrev_lock);
+         rwlock_init (result->fake_loclists_cu->split_lock);
+         mutex_init (result->fake_loclists_cu->src_lock);
+         mutex_init (result->fake_loclists_cu->str_off_base_lock);
+         mutex_init (result->fake_loclists_cu->intern_lock);
        }
     }
 
@@ -420,6 +430,11 @@ valid_p (Dwarf *result)
          result->fake_addr_cu->version = 5;
          result->fake_addr_cu->split = NULL;
          eu_search_tree_init (&result->fake_addr_cu->locs_tree);
+         rwlock_init (result->fake_addr_cu->abbrev_lock);
+         rwlock_init (result->fake_addr_cu->split_lock);
+         mutex_init (result->fake_addr_cu->src_lock);
+         mutex_init (result->fake_addr_cu->str_off_base_lock);
+         mutex_init (result->fake_addr_cu->intern_lock);
        }
     }
 
index 44240279a957579b6c1d3469028e3d9df7da9411..f391f9f927610368ebcb0b680fa88b5a924f7750 100644 (file)
@@ -39,7 +39,10 @@ dwarf_cfi_addrframe (Dwarf_CFI *cache, Dwarf_Addr address, Dwarf_Frame **frame)
   if (cache == NULL)
     return -1;
 
+  mutex_lock (cache->lock);
   struct dwarf_fde *fde = __libdw_find_fde (cache, address);
+  mutex_unlock (cache->lock);
+
   if (fde == NULL)
     return -1;
 
index 1628e448b0c9d11194dfa8d732b12b5780555163..979b11689edac09c466f1604333d7abab797a70e 100644 (file)
@@ -61,17 +61,19 @@ static void
 cu_free (void *arg)
 {
   struct Dwarf_CU *p = (struct Dwarf_CU *) arg;
+
   eu_search_tree_fini (&p->locs_tree, noop_free);
+  rwlock_fini (p->abbrev_lock);
+  rwlock_fini (p->split_lock);
+  mutex_fini (p->src_lock);
+  mutex_fini (p->str_off_base_lock);
+  mutex_fini (p->intern_lock);
 
   /* Only free the CU internals if its not a fake CU.  */
   if (p != p->dbg->fake_loc_cu && p != p->dbg->fake_loclists_cu
      && p != p->dbg->fake_addr_cu)
     {
       Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
-      rwlock_fini (p->abbrev_lock);
-      rwlock_fini (p->split_lock);
-      mutex_fini (p->src_lock);
-      mutex_fini (p->str_off_base_lock);
 
       /* Free split dwarf one way (from skeleton to split).  */
       if (p->unit_type == DW_UT_skeleton
index 07f998cd4f4327a80f1e3afc6a944473f795ca56..c18ee49959eb47cf89cb1ad7400da31801cfc488 100644 (file)
@@ -57,11 +57,13 @@ dwarf_frame_cfa (Dwarf_Frame *fs, Dwarf_Op **ops, size_t *nops)
 
     case cfa_expr:
       /* Parse the expression into internal form.  */
+      mutex_lock (fs->cache->lock);
       result = __libdw_intern_expression
        (NULL, fs->cache->other_byte_order,
         fs->cache->e_ident[EI_CLASS] == ELFCLASS32 ? 4 : 8, 4,
         &fs->cache->expr_tree, &fs->cfa_data.expr, false, false,
         ops, nops, IDX_debug_frame);
+      mutex_unlock (fs->cache->lock);
       break;
 
     case cfa_invalid:
index a6b7c4c1a373d4b657305df6b43d23f2d9b20d66..dbd7f1b258b441920596490af817fcda891a3959 100644 (file)
@@ -109,12 +109,16 @@ dwarf_frame_register (Dwarf_Frame *fs, int regno, Dwarf_Op ops_mem[3],
        block.data = (void *) p;
 
        /* Parse the expression into internal form.  */
-       if (__libdw_intern_expression (NULL,
-                                      fs->cache->other_byte_order,
-                                      address_size, 4,
-                                      &fs->cache->expr_tree, &block,
-                                      true, reg->rule == reg_val_expression,
-                                      ops, nops, IDX_debug_frame) < 0)
+       mutex_lock (fs->cache->lock);
+       int res = __libdw_intern_expression (NULL,
+                                            fs->cache->other_byte_order,
+                                            address_size, 4,
+                                            &fs->cache->expr_tree, &block,
+                                            true, reg->rule == reg_val_expression,
+                                            ops, nops, IDX_debug_frame);
+       mutex_unlock (fs->cache->lock);
+
+       if (res < 0)
          return -1;
        break;
       }
index a44971520442542ee2257e34ac8e915884073b07..893e3c7420d8717db44c89f57c70b745e4257996 100644 (file)
@@ -70,6 +70,7 @@ dwarf_getcfi (Dwarf *dbg)
       eu_search_tree_init (&cfi->cie_tree);
       eu_search_tree_init (&cfi->fde_tree);
       eu_search_tree_init (&cfi->expr_tree);
+      mutex_init (cfi->lock);
 
       cfi->ebl = NULL;
 
index ad1d46cae7fdcd02ce78a4aa2fab0d65f287adba..6d7296f8101374902a7cfed3e063363e81a3db58 100644 (file)
@@ -154,7 +154,7 @@ store_implicit_value (Dwarf *dbg, search_tree *cache, Dwarf_Op *op)
   block->addr = op;
   block->data = (unsigned char *) data;
   block->length = op->number;
-  if (unlikely (eu_tsearch (block, cache, loc_compare) == NULL))
+  if (unlikely (eu_tsearch_nolock (block, cache, loc_compare) == NULL))
     return 1;
   return 0;
 }
@@ -211,14 +211,18 @@ is_constant_offset (Dwarf_Attribute *attr,
     }
 
   /* Check whether we already cached this location.  */
+  mutex_lock (attr->cu->intern_lock);
   struct loc_s fake = { .addr = attr->valp };
-  struct loc_s **found = eu_tfind (&fake, &attr->cu->locs_tree, loc_compare);
+  struct loc_s **found = eu_tfind_nolock (&fake, &attr->cu->locs_tree, loc_compare);
 
   if (found == NULL)
     {
       Dwarf_Word offset;
       if (INTUSE(dwarf_formudata) (attr, &offset) != 0)
-       return -1;
+       {
+         mutex_unlock (attr->cu->intern_lock);
+         return -1;
+       }
 
       Dwarf_Op *result = libdw_alloc (attr->cu->dbg,
                                      Dwarf_Op, sizeof (Dwarf_Op), 1);
@@ -236,9 +240,10 @@ is_constant_offset (Dwarf_Attribute *attr,
       newp->loc = result;
       newp->nloc = 1;
 
-      found = eu_tsearch (newp, &attr->cu->locs_tree, loc_compare);
+      found = eu_tsearch_nolock (newp, &attr->cu->locs_tree, loc_compare);
     }
 
+  mutex_unlock (attr->cu->intern_lock);
   assert ((*found)->nloc == 1);
 
   if (llbuf != NULL)
@@ -267,7 +272,7 @@ __libdw_intern_expression (Dwarf *dbg, bool other_byte_order,
 
   /* Check whether we already looked at this list.  */
   struct loc_s fake = { .addr = block->data };
-  struct loc_s **found = eu_tfind (&fake, cache, loc_compare);
+  struct loc_s **found = eu_tfind_nolock (&fake, cache, loc_compare);
   if (found != NULL)
     {
       /* We already saw it.  */
@@ -656,7 +661,7 @@ __libdw_intern_expression (Dwarf *dbg, bool other_byte_order,
   newp->addr = block->data;
   newp->loc = result;
   newp->nloc = *listlen;
-  eu_tsearch (newp, cache, loc_compare);
+  eu_tsearch_nolock (newp, cache, loc_compare);
 
   /* We did it.  */
   return 0;
@@ -674,13 +679,17 @@ getlocation (struct Dwarf_CU *cu, const Dwarf_Block *block,
       return 0;
     }
 
-  return __libdw_intern_expression (cu->dbg, cu->dbg->other_byte_order,
-                                   cu->address_size, (cu->version == 2
-                                                      ? cu->address_size
-                                                      : cu->offset_size),
-                                   &cu->locs_tree, block,
-                                   false, false,
-                                   llbuf, listlen, sec_index);
+  mutex_lock (cu->intern_lock);
+  int res = __libdw_intern_expression (cu->dbg, cu->dbg->other_byte_order,
+                                      cu->address_size, (cu->version == 2
+                                                         ? cu->address_size
+                                                         : cu->offset_size),
+                                      &cu->locs_tree, block,
+                                      false, false,
+                                      llbuf, listlen, sec_index);
+  mutex_unlock (cu->intern_lock);
+
+  return res;
 }
 
 int
index f1c831fa9e42e6e0558373196cfe51818ce6ffe0..d7ed7b58137e3f7e65f4e987b97e76583ad6dcd4 100644 (file)
@@ -322,15 +322,29 @@ cache_op_table (Dwarf *dbg, int sec_index, Dwarf_Off macoff,
   if (found != NULL)
     return *found;
 
+  mutex_lock (dbg->macro_lock);
+
+  found = eu_tfind_nolock (&fake, &dbg->macro_ops_tree, macro_op_compare);
+  if (found != NULL)
+    {
+      mutex_unlock (dbg->macro_lock);
+      return *found;
+    }
+
   Dwarf_Macro_Op_Table *table = sec_index == IDX_debug_macro
     ? get_table_for_offset (dbg, macoff, startp, endp, cudie)
     : get_macinfo_table (dbg, macoff, cudie);
 
   if (table == NULL)
-    return NULL;
+    {
+      mutex_unlock (dbg->macro_lock);
+      return NULL;
+    }
+
+  Dwarf_Macro_Op_Table **ret = eu_tsearch_nolock (table, &dbg->macro_ops_tree,
+                                                 macro_op_compare);
+  mutex_unlock (dbg->macro_lock);
 
-  Dwarf_Macro_Op_Table **ret = eu_tsearch (table, &dbg->macro_ops_tree,
-                                       macro_op_compare);
   if (unlikely (ret == NULL))
     {
       __libdw_seterrno (DWARF_E_NOMEM);
index a5167805b43a42724e0719b9dff2792b11fcddaa..7f36c4a9202d188faebe2a1bbf977d01310127ec 100644 (file)
@@ -122,7 +122,8 @@ intern_fde (Dwarf_CFI *cache, const Dwarf_FDE *entry)
     fde->instructions += cie->fde_augmentation_data_size;
 
   /* Add the new entry to the search tree.  */
-  struct dwarf_fde **tres = eu_tsearch (fde, &cache->fde_tree, &compare_fde);
+  struct dwarf_fde **tres = eu_tsearch_nolock (fde, &cache->fde_tree,
+                                              &compare_fde);
   if (tres == NULL)
     {
       free (fde);
@@ -252,7 +253,8 @@ __libdw_find_fde (Dwarf_CFI *cache, Dwarf_Addr address)
   /* Look for a cached FDE covering this address.  */
 
   const struct dwarf_fde fde_key = { .start = address, .end = 0 };
-  struct dwarf_fde **found = eu_tfind (&fde_key, &cache->fde_tree, &compare_fde);
+  struct dwarf_fde **found = eu_tfind_nolock (&fde_key, &cache->fde_tree,
+                                             &compare_fde);
   if (found != NULL)
     return *found;
 
index 6c89858a3b37800c64b4677ab26c9f38342fdb07..6f6f777ee8d80760eedd88e3a9dee45ae6df7b2c 100644 (file)
@@ -64,6 +64,7 @@ __libdw_destroy_frame_cache (Dwarf_CFI *cache)
   eu_search_tree_fini (&cache->fde_tree, free_fde);
   eu_search_tree_fini (&cache->cie_tree, free_cie);
   eu_search_tree_fini (&cache->expr_tree, free_expr);
+  mutex_fini (cache->lock);
 
   if (cache->ebl != NULL && cache->ebl != (void *) -1l)
     ebl_closebackend (cache->ebl);
index de80cd4e551442ba459f370746b7fcb10d2d6010..25d53d31c7c84f2042cd73cfe3ff2223ace54ac0 100644 (file)
@@ -269,7 +269,7 @@ struct Dwarf
      __libdw_intern_next_unit.  */
   mutex_define(, dwarf_lock);
 
-  /* Synchronize access to dwarf_macro_getsrcfiles.  */
+  /* Synchronize access to dwarf_macro_getsrcfiles and cache_op_table.  */
   mutex_define(, macro_lock);
 
   /* Internal memory handling.  This is basically a simplified thread-local
@@ -471,6 +471,10 @@ struct Dwarf_CU
      Covers __libdw_str_offsets_base_off.  */
   mutex_define(, str_off_base_lock);
 
+  /* Synchronize access to is_constant_offset.  Should also be held
+     when calling __libdw_intern_expression with Dwarf_CU members.  */
+  mutex_define(, intern_lock);
+
   /* Memory boundaries of this CU.  */
   void *startp;
   void *endp;
@@ -949,8 +953,9 @@ extern int __libdw_visit_scopes (unsigned int depth,
                                 void *arg)
   __nonnull_attribute__ (2, 4) internal_function;
 
-/* Parse a DWARF Dwarf_Block into an array of Dwarf_Op's,
-   and cache the result (via tsearch).  */
+/* Parse a DWARF Dwarf_Block into an array of Dwarf_Op's, and cache the
+   result (via tsearch).  The owner of CACHE (typically a Dwarf_CU or
+   Dwarf_CFI_s) must hold a lock when calling this function.  */
 extern int __libdw_intern_expression (Dwarf *dbg,
                                      bool other_byte_order,
                                      unsigned int address_size,
index 0e4dcc3792466cad9ef99e3eda83cb07f48a9abd..592673439c488dbb20ad13b7203827017180ef8a 100644 (file)
@@ -181,6 +181,7 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
   rwlock_init (newp->split_lock);
   mutex_init (newp->src_lock);
   mutex_init (newp->str_off_base_lock);
+  mutex_init (newp->intern_lock);
 
   /* v4 debug type units have version == 4 and unit_type == DW_UT_type.  */
   if (debug_types)
@@ -240,8 +241,6 @@ struct Dwarf_CU *
 internal_function
 __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
 {
-  mutex_lock (dbg->dwarf_lock);
-
   search_tree *tree = v4_debug_types ? &dbg->tu_tree : &dbg->cu_tree;
   Dwarf_Off *next_offset
     = v4_debug_types ? &dbg->next_tu_offset : &dbg->next_cu_offset;
@@ -250,6 +249,12 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
   struct Dwarf_CU fake = { .start = start, .end = 0 };
   struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
   struct Dwarf_CU *result = NULL;
+  if (found != NULL)
+    return *found;
+
+  mutex_lock (dbg->dwarf_lock);
+
+  found = eu_tfind (&fake, tree, findcu_cb);
   if (found != NULL)
     {
       mutex_unlock (dbg->dwarf_lock);