Bug#759530: Patch to fix segfault in ldconfig
control: forwarded -1 https://sourceware.org/bugzilla/show_bug.cgi?id=18093
On 2015-02-25 13:49, Lennart Sorensen wrote:
> I looked at ways the aux-cache could cause a segfault, and given the
> file is mmap'd and has data offsets in it that are used as pointers
> without being checked it is not hard to see how a corrupt file could
> cause a segfault. The following patch makes the segfaults I was able
> to think of and create go away.
Thanks for your work, it's nice we have been able to understand the real
issue.
> I also have included an example corrupted aux-cache file
> (aux-cache-corrupt-soname-offset) which has a bad offset that causes
> a segfault.
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.
> There is another problem which I haven't solved but which is not a
> segfault. If you somehow truncate the aux-cache file by a bit and run
> the previous ldconfig without my patch, then you end up with a corrupted
> aux-cache where some entries do not have soname's even though they should,
> and that causes you to get messages like:
>
> /sbin/ldconfig.real: /lib/i386-linux-gnu/ is not a symbolic link
>
> /sbin/ldconfig.real: /usr/lib/i386-linux-gnu/ is not a symbolic link
>
> /sbin/ldconfig.real: /lib64/ is not a symbolic link
>
> /sbin/ldconfig.real: /usr/lib64/ is not a symbolic link
>
> /sbin/ldconfig.real: /libx32/ is not a symbolic link
>
> /sbin/ldconfig.real: /usr/libx32/ is not a symbolic link
>
> /sbin/ldconfig.real: /usr/lib/ is not a symbolic link
>
> /sbin/ldconfig.real: /usr/lib/i386-linux-gnu/i586/ is not a symbolic link
>
> /sbin/ldconfig.real: /usr/lib/i386-linux-gnu/i686/cmov/ is not a symbolic link
>
> Using ldconfig -i (and hence ignoring the corrupted aux-cache) makes
> that problem go away. To solve it would of course mean you have to
> not trust the cache which rather defeats the point of having the cache,
> so I don't know if that is worth trying to solve. It does not cause a
> segfault however.
>
> Using ldconfig -p to show the cache at that point has entries that are
> clearly wrong such as:
>
> ...
> day (libc6, OS ABI: Linux 2.6.32) => /lib/i386-linux-gnu/day
> __kernel_sigreturn (libc6,x32, OS ABI: Linux 3.4.0) => /libx32/__kernel_sigreturn
> X_2.6 (libc6) => /usr/lib/i386-linux-gnu/X_2.6
> LF (libc6) => /usr/lib/i386-linux-gnu/LF
> (libc6) => /usr/lib/
> (libc6, OS ABI: Linux 2.6.32) => /lib/i386-linux-gnu/
> (libc6, OS ABI: Linux 2.6.32) => /usr/lib/i386-linux-gnu/
> (libc6) => /lib/i386-linux-gnu/
> (libc6) => /usr/lib/i386-linux-gnu/
> (libc6) => /usr/lib/i386-linux-gnu/
> (libc6) => /usr/lib/i386-linux-gnu/
> (libc6) => /usr/lib/i386-linux-gnu/
> (libc6, OS ABI: Linux 2.6.32) => /lib/i386-linux-gnu/
> (libc6, OS ABI: Linux 2.6.32) => /usr/lib/i386-linux-gnu/
> (libc6,x32, OS ABI: Linux 3.4.0) => /libx32/
> (libc6,x32, OS ABI: Linux 3.4.0) => /usr/libx32/
> (libc6,x86-64, OS ABI: Linux 2.6.32) => /lib64/
> (libc6,x86-64, OS ABI: Linux 2.6.32) => /usr/lib64/
> (libc6, hwcap: 0x0008000000008000) => /usr/lib/i386-linux-gnu/i686/cmov/
> (libc6, hwcap: 0x0004000000000000) => /usr/lib/i386-linux-gnu/i586/
> (libc6) => /lib/i386-linux-gnu/
> (libc6) => /usr/lib/i386-linux-gnu/
> (libc6) => /usr/lib/
> � (libc6) => /lib/i386-linux-gnu/�
>
> The file aux-cache-missing-sonames shows this corrupted state.
>
> I hope the patch at least helps solve the worst part of the problem.
I think the idea behind the patch is correct, but it is not fully
correct (or at least it only fixes corner cases).
> 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?
> + || 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.
> {
> 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.
> + 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))
{
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);
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
Reply to: