Hi Mark,
On Thu, Mar 27, 2025 at 4:51 AM Mark Wielaard <m...@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 <ame...@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
> >
>
