Re: [PATCH v3 4/5] elf: Avoid RELATIVE relocs in __tunables_init

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 12/01/2021 14:22, Szabolcs Nagy via Libc-alpha wrote:
> With static pie linking pointers in the tunables list need
> RELATIVE relocs since the absolute address is not known at link
> time. We want to avoid relocations so the static pie self
> relocation can be done after tunables are initialized.
> 
> This is a simple fix that embeds the tunable strings into the
> tunable list instead of using pointers.  It is possible to have
> a more compact representation of tunables with some additional
> complexity in the generator and tunable parser logic.  Such
> optimization will be useful if the list of tunables grows.
> 
> There is still an issue that tunables_strdup allocates and the
> failure handling code path is sufficiently complex that it can
> easily have RELATIVE relocations.  It is possible to avoid the
> early allocation and only change environment variables in a
> setuid exe after relocations are processed.  But that is a
> bigger change and early failure is fatal anyway so it is not
> as critical to fix right away.

The _dl_fatal_printf is used quite earlier in the loader, does
still represent a potential failure? If so I think it would be
good to open a bug report to track it.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-tunable-types.h   |  4 ++--
>  elf/dl-tunables.c        |  2 +-
>  scripts/gen-tunables.awk | 12 +++++++++++-
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index 05d4958e1c..3fcc0806f5 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -59,7 +59,7 @@ typedef enum
>  /* A tunable.  */
>  struct _tunable
>  {
> -  const char *name;			/* Internal name of the tunable.  */
> +  const char name[TUNABLE_NAME_MAX];	/* Internal name of the tunable.  */
>    tunable_type_t type;			/* Data type of the tunable.  */
>    tunable_val_t val;			/* The value.  */
>    bool initialized;			/* Flag to indicate that the tunable is
> @@ -75,7 +75,7 @@ struct _tunable
>  					   target module if the value is
>  					   considered unsafe.  */
>    /* Compatibility elements.  */
> -  const char *env_alias;		/* The compatibility environment
> +  const char env_alias[TUNABLE_ALIAS_MAX]; /* The compatibility environment
>  					   variable name.  */
>  };
>  

Ok.

> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 9b4d737fb8..3845b2c04e 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -350,7 +350,7 @@ __tunables_init (char **envp)
>  
>  	  /* Skip over tunables that have either been set already or should be
>  	     skipped.  */
> -	  if (cur->initialized || cur->env_alias == NULL)
> +	  if (cur->initialized || cur->env_alias[0] == '\0')
>  	    continue;
>  
>  	  const char *name = cur->env_alias;

Ok.

> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index cda12ef62e..fa63e86d1a 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -12,6 +12,8 @@ BEGIN {
>    tunable=""
>    ns=""
>    top_ns=""
> +  max_name_len=0
> +  max_alias_len=0
>  }
>  
>  # Skip over blank lines and comments.
> @@ -57,11 +59,14 @@ $1 == "}" {
>        maxvals[top_ns,ns,tunable] = max_of[types[top_ns,ns,tunable]]
>      }
>      if (!env_alias[top_ns,ns,tunable]) {
> -      env_alias[top_ns,ns,tunable] = "NULL"
> +      env_alias[top_ns,ns,tunable] = "{0}"
>      }
>      if (!security_level[top_ns,ns,tunable]) {
>        security_level[top_ns,ns,tunable] = "SXID_ERASE"
>      }
> +    len = length(top_ns"."ns"."tunable)
> +    if (len > max_name_len)
> +      max_name_len = len
>  
>      tunable = ""
>    }
> @@ -109,6 +114,9 @@ $1 == "}" {
>    }
>    else if (attr == "env_alias") {
>      env_alias[top_ns,ns,tunable] = sprintf("\"%s\"", val)
> +    len = length(val)
> +    if (len > max_alias_len)
> +      max_alias_len = len
>    }
>    else if (attr == "security_level") {
>      if (val == "SXID_ERASE" || val == "SXID_IGNORE" || val == "NONE") {
> @@ -158,6 +166,8 @@ END {
>  
>    print "\n#ifdef TUNABLES_INTERNAL"
>    # Internal definitions.
> +  print "# define TUNABLE_NAME_MAX " (max_name_len + 1)
> +  print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
>    print "# include \"dl-tunable-types.h\""
>    # Finally, the tunable list.
>    print "static tunable_t tunable_list[] attribute_relro = {"
> 

Ok.