[PATCH] elf_strptr.c: Prevent deadlock and data race during section decompression
Aaron Merey
amerey@redhat.com
Mon Oct 19 21:48:02 GMT 2026
Hi Mark,
On Sun, Oct 18, 2026 at 6:38 PM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi Aaron,
>
> On Wed, Sep 30, 2026 at 03:06:51PM -0400, Aaron Merey wrote:
> > elf_strptr may attempt to call __libelf_decompress_elf while holding a
> > rdlock. This will cause a deadlock since __libelf_decompress_elf
> > calls elf_getdata which attempts to acquire the corresponding wrlock.
> > Additionally, elf_strptr.c:get_zdata modifies its Elf_Scn argument
> > while only holding a rdlock when a wrlock is needed.
>
> Interesting a fairly fundemental function like elf_strptr has multiple
> issues.
>
> > Fix this by making elf_strptr acquire the wrlock while decompressing
> > and by adding a __libelf_decompress_elf_wrlock function that assumes
> > the caller is already holding a wrlock.
>
> That sound like a good plan.
>
> > libelf/
> >
> > * elf_compress.c (decompress_elf): Renamed from
> > __libelf_decompress_elf with new parameter wrlocked indicating
> > whether the function was called with a wrlock held. Also calls
> > _wrlock variants of internal libelf functions when appropriate.
> > (__libelf_decompress_elf): Now calls decompress_elf
> > with wrlocked argument indicating no wrlock is held.
> > (__libelf_decompress_elf_wrlock): New function that
> > calls decompress_elf with wrlocked argument indicating a wrlock is
> > held.
> > * elf_getdata.c (getdata): Renamed from __elf_getdata_rdlock with
> > new wrlocked parameter indicating whether the function was called
> > with a wrlock held.
> > (__elf_getdata_rdlock): Now calls getdata with wrlocked
> > argument indicating no wrlock is held.
> > (__elf_getdata_wrlock): Now calls getdata with wrlocked
> > argument indicating a wrlock is held.
> > * elf_strptr.c (get_zdata_wrlock): Renamed from get_zdata. Now assumes
> > a wrlock is held when called.
> > (elf_strptr): Upgrades rdlock to wrlock before calling
> > get_zdata_wrlock.
> > * libelfP.h (__libelf_decompress_elf_wrlock): Add declaration.
>
> That are a lot of changes. It does seem they are all needed though.
>
> > Signed-off-by: Aaron Merey <amerey@redhat.com>
> > ---
> > libelf/elf_compress.c | 45 +++++++++++++++++++++++++++++++++++++------
> > libelf/elf_getdata.c | 41 ++++++++++++++++++++-------------------
> > libelf/elf_strptr.c | 43 +++++++++++++++++++++++++++++++++--------
> > libelf/libelfP.h | 3 +++
> > 4 files changed, 98 insertions(+), 34 deletions(-)
> >
> > diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
> > index b982510d..bb341dda 100644
> > --- a/libelf/elf_compress.c
> > +++ b/libelf/elf_compress.c
> > @@ -465,13 +465,30 @@ __libelf_decompress (int chtype, void *buf_in, size_t size_in, size_t size_out)
> > }
> > }
> >
> > -void *
> > -internal_function
> > -__libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
> > +static void *
> > +decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign,
> > + bool wrlocked)
> > {
> > GElf_Chdr chdr;
> > - if (gelf_getchdr (scn, &chdr) == NULL)
> > - return NULL;
> > +
> > + if (scn->elf->class == ELFCLASS32)
> > + {
> > + Elf32_Chdr *c
> > + = wrlocked ? __elf32_getchdr_wrlock (scn) : elf32_getchdr (scn);
> > + if (c == NULL)
> > + return NULL;
> > + chdr.ch_type = c->ch_type;
> > + chdr.ch_size = c->ch_size;
> > + chdr.ch_addralign = c->ch_addralign;
> > + }
> > + else
> > + {
> > + Elf64_Chdr *c
> > + = wrlocked ? __elf64_getchdr_wrlock (scn) : elf64_getchdr (scn);
> > + if (c == NULL)
> > + return NULL;
> > + chdr = *c;
> > + }
>
> OK, now a static function. Cannot just use gelf_getchrd anymore
> because needing to possibly call with/without lock function. Good
> translation from 32/64 to gelf data.
>
> >
> > bool unknown_compression = false;
> > if (chdr.ch_type != ELFCOMPRESS_ZLIB)
> > @@ -503,7 +520,8 @@ __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
> > is slightly inefficient when the raw data needs to be
> > converted since then we'll be converting the whole buffer and
> > not just Chdr. */
> > - Elf_Data *data = elf_getdata (scn, NULL);
> > + Elf_Data *data
> > + = wrlocked ? __elf_getdata_wrlock (scn, NULL) : elf_getdata (scn, NULL);
> > if (data == NULL)
> > return NULL;
>
> OK. Avoid deadlock by checking welocked and calling correct getdata.
>
> > @@ -520,6 +538,21 @@ __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
> > return buf_out;
> > }
> >
> > +void *
> > +internal_function
> > +__libelf_decompress_elf_wrlock (Elf_Scn *scn, size_t *size_out,
> > + size_t *addralign)
> > +{
> > + return decompress_elf (scn, size_out, addralign, true);
> > +}
> > +
> > +void *
> > +internal_function
> > +__libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
> > +{
> > + return decompress_elf (scn, size_out, addralign, false);
> > +}
>
> OK. Wrappers for new static function passing wrlocked flag.
>
> > /* Assumes buf is a malloced buffer. */
> > void
> > internal_function
> > diff --git a/libelf/elf_getdata.c b/libelf/elf_getdata.c
> > index 7c3ac043..fc66684b 100644
> > --- a/libelf/elf_getdata.c
> > +++ b/libelf/elf_getdata.c
> > @@ -471,13 +471,11 @@ __libelf_set_data_list_rdlock (Elf_Scn *scn, int wrlocked)
> > scn->data_list_rear = &scn->data_list;
> > }
> >
> > -Elf_Data *
> > -internal_function
> > -__elf_getdata_rdlock (Elf_Scn *scn, Elf_Data *data)
> > +static Elf_Data *
> > +getdata (Elf_Scn *scn, Elf_Data *data, int wrlocked)
> > {
>
> So this is assumed to be called with a lock held (either rdlock or
> wrlock), so maybe call get_data_lock?
I've merged this patch with getdata renamed to get_data_lock.
Aaron
>
> > Elf_Data *result = NULL;
> > Elf *elf;
> > - int locked = 0;
> >
> > if (scn == NULL)
> > return NULL;
> > @@ -539,13 +537,16 @@ __elf_getdata_rdlock (Elf_Scn *scn, Elf_Data *data)
> > /* If the data for this section was not yet initialized do it now. */
> > if (scn->data_read == 0)
> > {
> > - /* We cannot acquire a write lock while we are holding a read
> > - lock. Therefore give up the read lock and then get the write
> > - lock. But this means that the data could meanwhile be
> > - modified, therefore start the tests again. */
> > - rwlock_unlock (elf->lock);
> > - rwlock_wrlock (elf->lock);
> > - locked = 1;
> > + if (wrlocked == 0)
> > + {
> > + /* We cannot acquire a write lock while we are holding a read
> > + lock. Therefore give up the read lock and then get the write
> > + lock. But this means that the data could meanwhile be
> > + modified, therefore start the tests again. */
> > + rwlock_unlock (elf->lock);
> > + rwlock_wrlock (elf->lock);
> > + wrlocked = 1;
> > + }
> >
> > /* Read the data from the file. There is always a file (or
> > memory region) associated with this descriptor since
> > @@ -559,7 +560,7 @@ __elf_getdata_rdlock (Elf_Scn *scn, Elf_Data *data)
> > empty in case the section has size zero (for whatever reason).
> > Now create the converted data in case this is necessary. */
> > if (scn->data_list_rear == NULL)
> > - __libelf_set_data_list_rdlock (scn, locked);
> > + __libelf_set_data_list_rdlock (scn, wrlocked);
> >
> > /* Return the first data element in the list. */
> > result = &scn->data_list.data.d;
> > @@ -585,15 +586,15 @@ elf_getdata (Elf_Scn *scn, Elf_Data *data)
> >
> > Elf_Data *
> > internal_function
> > -__elf_getdata_wrlock (Elf_Scn *scn, Elf_Data *data)
> > +__elf_getdata_rdlock (Elf_Scn *scn, Elf_Data *data)
> > {
> > - Elf_Data *result;
> > -
> > - if (scn == NULL)
> > - return NULL;
> > -
> > - result = __elf_getdata_rdlock (scn, data);
> > + return getdata (scn, data, 0);
> > +}
>
> OK, new rdlock wrapper.
>
> > - return result;
> > +Elf_Data *
> > +internal_function
> > +__elf_getdata_wrlock (Elf_Scn *scn, Elf_Data *data)
> > +{
> > + return getdata (scn, data, 1);
> > }
> > INTDEF(elf_getdata)
>
> OK, new internal_function implementation.
>
> > diff --git a/libelf/elf_strptr.c b/libelf/elf_strptr.c
> > index 7be7f5e8..f0a7d7ea 100644
> > --- a/libelf/elf_strptr.c
> > +++ b/libelf/elf_strptr.c
> > @@ -40,10 +40,10 @@
> >
> >
> > static void *
> > -get_zdata (Elf_Scn *strscn)
> > +get_zdata_wrlock (Elf_Scn *strscn)
> > {
> > size_t zsize, zalign;
> > - void *zdata = __libelf_decompress_elf (strscn, &zsize, &zalign);
> > + void *zdata = __libelf_decompress_elf_wrlock (strscn, &zsize, &zalign);
> > if (zdata == NULL)
> > return NULL;
>
> OK, rename function to indicate we assume wrlock is taken and call
> __libelf_decompress_elf_wrlock function variant.
>
> > @@ -100,6 +100,7 @@ elf_strptr (Elf *elf, size_t idx, size_t offset)
> > }
> > }
> >
> > + int wrlocked = 0;
> > size_t sh_size = 0;
> > if (elf->class == ELFCLASS32)
> > {
> > @@ -115,8 +116,19 @@ elf_strptr (Elf *elf, size_t idx, size_t offset)
> > sh_size = shdr->sh_size;
> > else
> > {
> > - if (strscn->zdata_base == NULL && get_zdata (strscn) == NULL)
> > - goto out;
> > + if (strscn->zdata_base == NULL)
> > + {
> > + rwlock_unlock (elf->lock);
> > + rwlock_wrlock (elf->lock);
> > + wrlocked = 1;
> > +
> > + /* Skip decompression if it occurred while grabbing
> > + the wrlock. */
> > + if (strscn->zdata_base == NULL
> > + && get_zdata_wrlock (strscn) == NULL)
> > + goto out;
> > + }
> > +
>
> OK, double check and call new get_zdata_wrlock as above.
>
> > sh_size = strscn->zdata_size;
> > }
> >
> > @@ -141,8 +153,19 @@ elf_strptr (Elf *elf, size_t idx, size_t offset)
> > sh_size = shdr->sh_size;
> > else
> > {
> > - if (strscn->zdata_base == NULL && get_zdata (strscn) == NULL)
> > - goto out;
> > + if (strscn->zdata_base == NULL)
> > + {
> > + rwlock_unlock (elf->lock);
> > + rwlock_wrlock (elf->lock);
> > + wrlocked = 1;
> > +
> > + /* Skip decompression if it occurred while grabbing
> > + the wrlock. */
> > + if (strscn->zdata_base == NULL
> > + && get_zdata_wrlock (strscn) == NULL)
> > + goto out;
> > + }
> > +
>
> OK, double check and call new get_zdata_wrlock as above.
>
> > sh_size = strscn->zdata_size;
> > }
> >
> > @@ -156,8 +179,12 @@ elf_strptr (Elf *elf, size_t idx, size_t offset)
> >
> > if (strscn->rawdata_base == NULL && ! strscn->data_read)
> > {
> > - rwlock_unlock (elf->lock);
> > - rwlock_wrlock (elf->lock);
> > + if (wrlocked == 0)
> > + {
> > + rwlock_unlock (elf->lock);
> > + rwlock_wrlock (elf->lock);
> > + wrlocked = 1;
> > + }
>
> Setting wrlocked here, but it is never used after this check.
>
> > if (strscn->rawdata_base == NULL && ! strscn->data_read
> > /* Read the section data. */
> > && __libelf_set_rawdata_wrlock (strscn) != 0)
> > diff --git a/libelf/libelfP.h b/libelf/libelfP.h
> > index 2403d796..52767aa5 100644
> > --- a/libelf/libelfP.h
> > +++ b/libelf/libelfP.h
> > @@ -602,6 +602,9 @@ extern void * __libelf_decompress (int chtype, void *buf_in, size_t size_in,
> > extern void * __libelf_decompress_elf (Elf_Scn *scn,
> > size_t *size_out, size_t *addralign)
> > internal_function;
> > +extern void * __libelf_decompress_elf_wrlock (Elf_Scn *scn, size_t *size_out,
> > + size_t *addralign)
> > + internal_function;
>
> Declaring a new internal function. OK.
>
> Some naming nitpicks, but this looks good as is.
>
> Thanks,
>
> Mark
>
More information about the Elfutils-devel
mailing list