[COMMITTED] libdw/libdw_findcu.c: Fix TOCTOU race condition in __libdw_findcu

[Aaron Merey]

Hi Mark,

On Thu, Mar 27, 2025 at 4:51 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi Aaron,
>
> On Thu, Mar 27, 2025 at 12:07:18AM -0400, Aaron Merey wrote:
> > Ensure that dwarf_lock is held before accessing next_tu_offset and
> > next_cu_offset.
> >
> > This fixes a TOCTOU bug in __libdw_findcu that causes NULL to be
> > incorrectly returned.
>
> Could you explain what the issue is in a few more words?
>
> I am not sure I fully follow what the lock protects here precisely.
> The patch and the mention of TOCTOU seem to indicate it is about this in particular:
>
>   struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
>   if (found != NULL)
>     return *found;
>
> Does that mean that despite eu_tfind internally taking a lock on the
> tree lock, the returned result can still change after the call? If
> that is the case how exactly does that happen?

Multiple threads might call eu_tfind before the desired Dwarf_CU has
been added to the search_tree. In this case `found == NULL` for both
threads. The first thread to grab the mutex will successfully find the
Dwarf_CU, add it to the search_tree and return it. For the second
thread, `if (start < *next_offset)` will evaluate to true because the
first thread will have already updated *next_offset. __libdw_seterrno
(DWARF_E_INVALID_DWARF) gets set and NULL is returned.

>
> Or isn't this the part that needs guarding?

Yes this part needs guarding. The mutex_lock could be moved from the
beginning of __libdw_findcu to immediately before the call to
eu_tfind.

> Asking because if it is this eu_tfind check return != NULL construct
> then there are more places that might need extra guarding (or we have
> to write better eu-search wrappers?)

Yes there are other eu_tfind calls that are susceptible to this bug.
If eu_tfind returns NULL, a lock should continue to be held until the
thread has a chance to add the result to the search_tree. I will work
on a patch to improve the eu-search wrappers.

Aaron

>
> Thanks,
>
> Mark
>
> > Signed-off-by: Aaron Merey <amerey@redhat.com>
> > ---
> >  libdw/libdw_findcu.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> > index 8805af9b..0e4dcc37 100644
> > --- a/libdw/libdw_findcu.c
> > +++ b/libdw/libdw_findcu.c
> > @@ -240,6 +240,8 @@ 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;
> > @@ -249,9 +251,10 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
> >    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);
> > +    {
> > +      mutex_unlock (dbg->dwarf_lock);
> > +      return *found;
> > +    }
> >
> >    if (start < *next_offset)
> >      __libdw_seterrno (DWARF_E_INVALID_DWARF);
> > --
> > 2.49.0
> >
>