This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][BUG 18093] Fix ldconfig segmentation fault with corrupted cache


On 03/08/2015 04:46 PM, Aurelien Jarno wrote:
> ldconfig is using an aux-cache to speed up the ld.so.cache update. It
> is read by mmaping the file to a structure which contains data offsets
> used as pointers. As they are not checked, it is not hard to get
> ldconfig to segfault with a corrupted file. This happens for instance if
> the file is truncated, which is common following a filesystem check
> following a system crash.

That makes sense.
 
> This can be reproduced for example by truncating the file to roughly
> half of it's size.
> 
> There is already in some code in elf/cache.c (load_aux_cache) to check
> for a corrupted aux cache, but it happens to be broken and not enough.
> The test (aux_cache->nlibs >= aux_cache_size) compares the number of
> libs entry with the cache size. It's a non sense, as it basically
> assumes that each library entry is a 1 byte... Instead the patch below
> computes the theoretical cache size using the headers and compares it
> to the real size.

This is OK.

> Another corruption can happen if the pointers to the string table is
> corrupted though in that case it means that the file has been more than
> just truncated. The patch below ignores entries pointing outside of the
> string table, they will be added with the current ldconfig run.

This is not OK, since it requires having incorrectly modified the file
which we assume cannot happen and slows down the cache access.

> (This patch is based on an initial patch from Lennart Sorensen).
> 
> 
> 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.
> 
> 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))

I'm not happy to have "!=" in the event the file is larger, but has
zero padding, thus I would like to see `aux_cache_size >=`.

>      {
>        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);

Drop this, we don't need it. You've not shown how the file could
get corrupted, and therefore this check is not needed and slows
down cache loading.

>  
>    munmap (aux_cache, aux_cache_size);
>    close (fd);
> 

Please repost v2 for review.

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]