[v3,4/5] elf: Avoid RELATIVE relocs in __tunables_init
Commit Message
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.
---
elf/dl-tunable-types.h | 4 ++--
elf/dl-tunables.c | 2 +-
scripts/gen-tunables.awk | 12 +++++++++++-
3 files changed, 14 insertions(+), 4 deletions(-)
Comments
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.
@@ -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. */
};
@@ -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;
@@ -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 = {"