[PATCH 22/28] elf: Add extension mechanism to ld.so.cache

[Adhemerval Zanella]

On 01/10/2020 13:33, Florian Weimer via Libc-alpha wrote:
> A previously unused new-format header field is used to record
> the address of an extension directory.
> 
> This change adds a demo extension which records the version of
> ldconfig which builds a file.

Looks good in general, some comments below.

> ---
>  elf/cache.c                |  89 +++++++++++++++++++++++++++
>  sysdeps/generic/dl-cache.h | 123 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 211 insertions(+), 1 deletion(-)
> 
> diff --git a/elf/cache.c b/elf/cache.c
> index e0aa616352..3a02a4070a 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c
> @@ -15,6 +15,7 @@
>     You should have received a copy of the GNU General Public License
>     along with this program; if not, see <https://www.gnu.org/licenses/>.  */
>  
> +#include <assert.h>
>  #include <errno.h>
>  #include <error.h>
>  #include <dirent.h>
> @@ -33,6 +34,7 @@
>  
>  #include <ldconfig.h>
>  #include <dl-cache.h>
> +#include <version.h>
>  
>  struct cache_entry
>  {
> @@ -161,6 +163,21 @@ check_new_cache (struct cache_file_new *cache)
>      error (EXIT_FAILURE, 0, _("Cache file has wrong endianness.\n"));
>  }
>  
> +/* Print the extension information at the cache at start address
> +   FILE_BASE, of ltength FILE_SIZE bytes.  The new-format cache header

s/ltength/length

> +   is at CACHE, and the file name for diagnostics is CACHE_NAME.  */
> +static void
> +print_extensions (struct cache_extension_all_loaded *ext)
> +{
> +  if (ext->sections[cache_extension_tag_generator].base != NULL)
> +    {
> +      fputs (_("Cache generated by: "), stdout);
> +      fwrite (ext->sections[cache_extension_tag_generator].base, 1,
> +	      ext->sections[cache_extension_tag_generator].size, stdout);
> +      putchar ('\n');
> +    }
> +}
> +

Ok.  Will be the extension tag data always comprised of ascii printable
characters? 

>  /* Print the whole cache file, if a file contains the new cache format
>     hidden in the old one, print the contents of the new format.  */
>  void
> @@ -250,6 +267,11 @@ print_cache (const char *cache_name)
>      }
>    else if (format == 1)
>      {
> +      struct cache_extension_all_loaded ext;
> +      if (!cache_extension_load (cache_new, cache, cache_size, &ext))
> +	error (EXIT_FAILURE, 0,
> +	       _("Malformed extension data in cache file %s\n"), cache_name);
> +
>        printf (_("%d libs found in cache `%s'\n"),
>  	      cache_new->nlibs, cache_name);
>  

Ok.

> @@ -260,6 +282,7 @@ print_cache (const char *cache_name)
>  		     cache_new->libs[i].osversion,
>  		     cache_new->libs[i].hwcap,
>  		     cache_data + cache_new->libs[i].value);
> +      print_extensions (&ext);
>      }
>    /* Cleanup.  */
>    munmap (cache, cache_size);

Ok.

> @@ -301,6 +324,45 @@ compare (const struct cache_entry *e1, const struct cache_entry *e2)
>    return res;
>  }
>  
> +/* Size of the cache extension directory.  All tags are assumed to be
> +   present.  */
> +enum
> +  {
> +   cache_extension_size = (offsetof (struct cache_extension, sections)
> +			   + (cache_extension_count
> +			      * sizeof (struct cache_extension_section)))
> +  };
> +
> +/* Write the cache extensions to FD.  The extension directory is
> +   assumed to be located at CACHE_EXTENSION_OFFSET.  */
> +static void
> +write_extensions (int fd, uint32_t cache_extension_offset)
> +{
> +  assert ((cache_extension_offset % 4) == 0);

Maybe a proper error msg instead of an assert here?

> +
> +  struct cache_extension *ext = xmalloc (cache_extension_size);
> +  ext->magic = cache_extension_magic;
> +  ext->count = cache_extension_count;
> +
> +  for (int i = 0; i < cache_extension_count; ++i)
> +    {
> +      ext->sections[i].tag = i;
> +      ext->sections[i].flags = 0;
> +    }
> +
> +  const char *generator
> +    = "ldconfig " PKGVERSION RELEASE " release version " VERSION;
> +  ext->sections[cache_extension_tag_generator].offset
> +    = cache_extension_offset + cache_extension_size;
> +  ext->sections[cache_extension_tag_generator].size = strlen (generator);
> +
> +  if (write (fd, ext, cache_extension_size) != cache_extension_size
> +      || write (fd, generator, strlen (generator)) != strlen (generator))
> +    error (EXIT_FAILURE, errno, _("Writing of cache extension data failed"));
> +
> +  free (ext);
> +}
> +

Ok.

>  /* Save the contents of the cache.  */
>  void
>  save_cache (const char *cache_name)
> @@ -435,6 +497,25 @@ save_cache (const char *cache_name)
>        && idx_old < cache_entry_old_count)
>      file_entries->libs[idx_old] = file_entries->libs[idx_old - 1];
>  
> +  /* Compute the location of the extension directory.  This
> +     implementation puts the directory after the string table.  The
> +     size computation matches the write calls below.  The extension
> +     directory does not exist with format 0, so the value does not
> +     matter.  */
> +  uint32_t extension_offset = 0;
> +  if (opt_format != 2)
> +    extension_offset += file_entries_size;
> +  if (opt_format != 0)
> +    {
> +      if (opt_format != 2)
> +	extension_offset += pad;
> +      extension_offset += file_entries_new_size;
> +    }

Ok, although I think we should be good move the 'opt_format' definition to
a proper enumeration.

> +  extension_offset += total_strlen;
> +  extension_offset = roundup (extension_offset, 4); /* Provide alignment.  */
> +  if (opt_format != 0)
> +    file_entries_new->extension_offset = extension_offset;
> +
>    /* Write out the cache.  */
>  
>    /* Write cache first to a temporary file and rename it later.  */

Ok.

> @@ -473,6 +554,14 @@ save_cache (const char *cache_name)
>    if (write (fd, strings, total_strlen) != (ssize_t) total_strlen)
>      error (EXIT_FAILURE, errno, _("Writing of cache data failed"));
>  
> +  if (opt_format != 0)
> +    {
> +      /* Align file position to 4.  */
> +      off64_t old_offset = lseek64 (fd, extension_offset, SEEK_SET);
> +      assert ((unsigned long long int) (extension_offset - old_offset) < 4);

Same as before, should we add a proper error in this case?

> +      write_extensions (fd, extension_offset);
> +    }
> +
>    /* Make sure user can always read cache file */
>    if (chmod (temp_name, S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR))
>      error (EXIT_FAILURE, errno,

Ok.

> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
> index 1b04211f6b..b154740da9 100644
> --- a/sysdeps/generic/dl-cache.h
> +++ b/sysdeps/generic/dl-cache.h
> @@ -21,7 +21,9 @@
>  
>  #include <endian.h>
>  #include <stdbool.h>
> +#include <stddef.h>
>  #include <stdint.h>
> +#include <string.h>
>  
>  #ifndef _DL_CACHE_DEFAULT_ID
>  # define _DL_CACHE_DEFAULT_ID	3
> @@ -115,7 +117,11 @@ struct cache_file_new
>  
>    uint8_t padding_unsed[3];	/* Not used, for future extensions.  */
>  
> -  uint32_t unused[4];		/* Leave space for future extensions
> +  /* File offset of the extension directory.  See struct
> +     cache_extension below.  Must be a multiple of four.  */
> +  uint32_t extension_offset;
> +
> +  uint32_t unused[3];		/* Leave space for future extensions
>  				   and align to 8 byte boundary.  */
>    struct file_entry_new libs[0]; /* Entries describing libraries.  */
>    /* After this the string table of size len_strings is found.	*/
> @@ -134,6 +140,121 @@ cache_file_new_matches_endian (const struct cache_file_new *cache)
>  }
>  
>  

Ok.

> +/* Randomly chosen magic value, which allows for additional
> +   consistency verification.  */
> +enum { cache_extension_magic = (uint32_t) -358342284 };
> +
> +/* Tag values for different kinds of extension sections.  Similar to
> +   SHT_* constants.  */
> +enum cache_extension_tag
> +  {
> +   /* Array of bytes containing the glibc version that generated this
> +      cache file.  */
> +   cache_extension_tag_generator,
> +
> +   /* Total number of known cache extension tags.  */
> +   cache_extension_count
> +  };
> +
> +/* Element in the array following struct cache_extension.  Similar to
> +   an ELF section header.  */
> +struct cache_extension_section
> +{
> +  /* Type of the extension section.  A enum cache_extension_tag value.  */
> +  uint32_t tag;
> +
> +  /* Extension-specific flags.  Currently generated as zero.  */
> +  uint32_t flags;
> +
> +  /* Offset from the start of the file for the data in this extension
> +     section.  Specific extensions can have alignment constraints.  */
> +  uint32_t offset;
> +
> +  /* Length in bytes of the extension data.  Specific extensions may
> +     have size requirements.  */
> +  uint32_t size;
> +};

Ok.

> +
> +/* The extension directory in the cache.  An array of struct
> +   cache_extension_section entries.  */
> +struct cache_extension
> +{
> +  uint32_t magic;		/* Always cache_extension_magic.  */
> +  uint32_t count;		/* Number of following entries.  */
> +
> +  /* count section descriptors of type struct cache_extension_section
> +     follow.  */
> +  struct cache_extension_section sections[];
> +};
> +

Ok.

> +/* A relocated version of struct cache_extension_section.  */
> +struct cache_extension_loaded
> +{
> +  /* Address and size of this extension section.  base is NULL if the
> +     section is missing from the file.  */
> +  const void *base;
> +  size_t size;
> +
> +  /* Flags from struct cache_extension_section.  */
> +  uint32_t flags;
> +};
> +
> +/* All supported extension sections, relocated.  Filled in by
> +   cache_extension_load below.  */
> +struct cache_extension_all_loaded
> +{
> +  struct cache_extension_loaded sections[cache_extension_count];
> +};
> +

Ok.

> +static bool __attribute__ ((unused))

Maybe use inline and let the compiler decide? Or the function is
really duplicate in a lot of places?

> +cache_extension_load (const struct cache_file_new *cache,
> +		      const void *file_base, size_t file_size,
> +		      struct cache_extension_all_loaded *loaded)
> +{
> +  memset (loaded, 0, sizeof (*loaded));
> +  if (cache->extension_offset == 0)
> +    /* No extensions present.  This is not a format error.  */
> +    return true;
> +  if ((cache->extension_offset % 4) != 0)
> +    /* Extension offset is misaligned.  */
> +    return false;
> +  size_t size_tmp;
> +  if (__builtin_add_overflow (cache->extension_offset,
> +			      sizeof (struct cache_extension), &size_tmp)
> +      || size_tmp > file_size)
> +    /* Extension extends beyond the end of the file.  */
> +    return false;
> +  const struct cache_extension *ext = file_base + cache->extension_offset;

Maybe we should add an alignment check for 'file_base' as well (to
avoid unaligned struct member deference)?

> +  if (ext->magic != cache_extension_magic)
> +    return false;
> +  if (__builtin_mul_overflow (ext->count,
> +			      sizeof (struct cache_extension_section),
> +			      &size_tmp)
> +      || __builtin_add_overflow (cache->extension_offset
> +				 + sizeof (struct cache_extension), size_tmp,
> +				 &size_tmp)
> +      || size_tmp > file_size)
> +    /* Extension array extends beyond the end of the file.  */
> +    return false;
> +  for (uint32_t i = 0; i < ext->count; ++i)
> +    {
> +      if (__builtin_add_overflow (ext->sections[i].offset,
> +				  ext->sections[i].size, &size_tmp)
> +	  || size_tmp > file_size)
> +	/* Extension data extends beyond the end of the file.  */
> +	return false;
> +
> +      uint32_t tag = ext->sections[i].tag;
> +      if (tag >= cache_extension_count)
> +	/* Tag is out of range and unrecognized.  */
> +	continue;
> +      loaded->sections[tag].base = file_base + ext->sections[i].offset;
> +      loaded->sections[tag].size = ext->sections[i].size;
> +      loaded->sections[tag].flags = ext->sections[i].flags;
> +    }
> +  return true;
> +}
> +
>  /* Used to align cache_file_new.  */
>  #define ALIGN_CACHE(addr)				\
>  (((addr) + __alignof__ (struct cache_file_new) -1)	\
> 

Ok.