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 <[email protected]> > --- > 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? > 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
