Bug#759530: Patch to fix segfault in ldconfig
On Sun, Mar 08, 2015 at 09:48:50PM +0100, Aurelien Jarno wrote:
> Thanks for your work, it's nice we have been able to understand the real
> issue.
Well I thing the best summary of the problem is that the aux-cache
handling code is awful and doesn't check anything before using the
incoming data as a source for pointers. Corrupt aux-cache files was
not even remotely considered. Safest option right now would be to disable
the use of the aux-cache if you want to avoid segfaults in ldconfig.
> Unfortunately they doesn't seem to work here. That said I have been able
> to reproduce the problem by truncating/changing the aux cache manually.
Well it segfaulted on my amd64 system when I tried that file. Of course
it may need to have the same set of libraries installed as I have.
> I think the idea behind the patch is correct, but it is not fully
> correct (or at least it only fixes corner cases).
I don't doubt I missed some of the problem cases. Seems the best answer
would be to redesign it with a checksum and a size indicator or something
else that would catch corruption in general.
> > diff -ur --exclude debian --exclude build-tree glibc-2.19.ori/elf/cache.c glibc-2.19/elf/cache.c
> > --- glibc-2.19.ori/elf/cache.c 2015-02-25 16:24:59.000000000 +0000
> > +++ glibc-2.19/elf/cache.c 2015-02-25 17:42:18.000000000 +0000
> > @@ -699,7 +699,8 @@
> > if (aux_cache == MAP_FAILED
> > || aux_cache_size < sizeof (struct aux_cache_file)
> > || memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1)
> > - || aux_cache->nlibs >= aux_cache_size)
> > + || sizeof(struct aux_cache_file) + (aux_cache->nlibs - 1) * sizeof(struct aux_cache_file_entry) >= aux_cache_size
>
> Why using (aux_cache->nlibs - 1) and not directly aux_cache->nlibs here?
Hmm, I seem to have misread the way the structure is defined, and thought
it always had one aux_cache_file_entry in the aux_cache_file struct,
but no it has 0 of them.
> > + || aux_cache->nlibs * sizeof(struct aux_cache_file_entry) + aux_cache->libs[aux_cache->nlibs - 1].soname >= aux_cache_size)
>
> The best to catch all cases here is to compute the theoretical size of
> the file using the headers and comparing it to the real one.
Well the filenames seem to be just stored at the end of the other data
with pointers to it (which is where most of the chances for segfaults
occur). The format doesn't seem to actually have anything to cover the
size of that pile of strings. There is really no way to know what the
filesize should be given the end is just a heap of c strings. Did I
miss something? You seem to be using anpother header thing that mentions
the string lengths. Maybe I missed that existing. That would be useful.
> > {
> > close (fd);
> > init_aux_cache ();
> > @@ -712,12 +713,14 @@
> > const char *aux_cache_data
> > = (const char *) &aux_cache->libs[aux_cache->nlibs];
> > for (unsigned int i = 0; i < aux_cache->nlibs; ++i)
> > - insert_to_aux_cache (&aux_cache->libs[i].id,
> > - aux_cache->libs[i].flags,
> > - aux_cache->libs[i].osversion,
> > - aux_cache->libs[i].soname == 0
> > - ? NULL : aux_cache_data + aux_cache->libs[i].soname,
> > - 0);
> > + /* Only use entries with sane offsets */
> > + if (aux_cache->libs[i].soname < aux_cache_size)
>
> The check is incorrect here, the address in aux_cache->libs[i].soname is
> relative to aux_cache_data, so it probably catches very few cases.
Well it catches any where the offset is corrupt by a larger value
(doesn't have to be that large either). But yes I did get that check
slightly wrong. I think I changed it a few times while working out what
the code was trying to do with the data structure.
> > + insert_to_aux_cache (&aux_cache->libs[i].id,
> > + aux_cache->libs[i].flags,
> > + aux_cache->libs[i].osversion,
> > + aux_cache->libs[i].soname == 0
> > + ? NULL : aux_cache_data + aux_cache->libs[i].soname,
> > + 0);
> >
> > munmap (aux_cache, aux_cache_size);
> > close (fd);
>
>
> Please find a new patch below. I have submitted it upstream as bz 18093.
> I am planning to wait for upstream answer or comment for other people a
> few days. I'll then prepare an upload fixing this bug.
>
> diff --git a/ChangeLog b/ChangeLog
> index 4a5cd16..5086267 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2015-03-08 Aurelien Jarno <aurelien@aurel32.net>
> +
> + [BZ #18093]
> + * elf/cache.c (load_aux_cache): Regenerate the cache if it has the
> + wrong size. Ignore entries pointing outside of the mmaped memory.
> +
> 2015-03-08 Paul Pluzhnikov <ppluzhnikov@google.com>
>
> [BZ #16734]
> diff --git a/elf/cache.c b/elf/cache.c
> index 1732268..9131e08 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c
> @@ -698,7 +698,9 @@ load_aux_cache (const char *aux_cache_name)
> if (aux_cache == MAP_FAILED
> || aux_cache_size < sizeof (struct aux_cache_file)
> || memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1)
> - || aux_cache->nlibs >= aux_cache_size)
> + || aux_cache_size != (sizeof(struct aux_cache_file) +
> + aux_cache->nlibs * sizeof(struct aux_cache_file_entry) +
> + aux_cache->len_strings))
Yeah that looks right.
> {
> close (fd);
> init_aux_cache ();
> @@ -711,12 +713,13 @@ load_aux_cache (const char *aux_cache_name)
> const char *aux_cache_data
> = (const char *) &aux_cache->libs[aux_cache->nlibs];
> for (unsigned int i = 0; i < aux_cache->nlibs; ++i)
> - insert_to_aux_cache (&aux_cache->libs[i].id,
> - aux_cache->libs[i].flags,
> - aux_cache->libs[i].osversion,
> - aux_cache->libs[i].soname == 0
> - ? NULL : aux_cache_data + aux_cache->libs[i].soname,
> - 0);
> + if (aux_cache->libs[i].soname < aux_cache->len_strings)
> + insert_to_aux_cache (&aux_cache->libs[i].id,
> + aux_cache->libs[i].flags,
> + aux_cache->libs[i].osversion,
> + aux_cache->libs[i].soname == 0
> + ? NULL : aux_cache_data + aux_cache->libs[i].soname,
> + 0);
>
> munmap (aux_cache, aux_cache_size);
> close (fd);
I think that is at least a big improvement. Not sure what would happen
if the null terminator on the last string got lost. Hmm. I didn't
test that.
Of course none of this solves the problem of the ld.so.cache being corrupt
already because the old ldconfig was run with a corrupt file but without
a segfault. The only fix for that would be a sane check for corruption,
or not using the cache at all.
--
Len Sorensen
Reply to: