This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Add more checks for valid ld.so.cache file (bug 18093)
- From: Florian Weimer <fweimer at redhat dot com>
- To: Andreas Schwab <schwab at suse dot de>
- Cc: libc-alpha at sourceware dot org
- Date: Tue, 23 Oct 2018 14:27:14 +0200
- Subject: Re: [PATCH] Add more checks for valid ld.so.cache file (bug 18093)
- References: <mvm7ei8swnb.fsf@suse.de>
* Andreas Schwab:
> [BZ #18093]
> * elf/dl-cache.c (_dl_load_cache_lookup): Check for truncated old
> format cache.
> * elf/cache.c (print_cache): Likewise.
> ---
> elf/cache.c | 5 +++++
> elf/dl-cache.c | 5 ++++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/elf/cache.c b/elf/cache.c
> index e63979da7d..83de25484b 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c
> @@ -199,6 +199,11 @@ print_cache (const char *cache_name)
> }
> else
> {
> + /* Check for overflow. */
> + if ((cache_size - sizeof (struct cache_file)) / sizeof (struct file_entry)
> + < cache->nlibs)
> + error (EXIT_FAILURE, 0, _("File is not a cache file.\n"));
> +
> size_t offset = ALIGN_CACHE (sizeof (struct cache_file)
> + (cache->nlibs
> * sizeof (struct file_entry)));
I think the “Check for overflow” are misleading because you are checking
for file truncation *and* avoiding an overflow in the check.
> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 6ee5153ff9..0f5d035213 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -204,7 +204,10 @@ _dl_load_cache_lookup (const char *name)
> - only the new format
> The following checks if the cache contains any of these formats. */
> if (file != MAP_FAILED && cachesize > sizeof *cache
> - && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0)
> + && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0
> + /* Check for overflow. */
> + && ((cachesize - sizeof *cache) / sizeof (struct file_entry)
> + >= ((struct cache_file *) file)->nlibs))
Should the new check be nested inside the if statement, so that we do
not fall through to the CACHEMAGIC_VERSION_NEW check?
Thanks,
Florian