| Message ID | 20240307214032.2773074-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] elf/rtld: Count skipped environment variables for enable_secure | expand |
Ping On Thu, Mar 7, 2024 at 4:40 PM Joe Simmons-Talbott <josimmon@redhat.com> wrote: > > When using the glibc.rtld.enable_secure tunable we need to keep track of > the count of environment variables we skip due to __libc_enable_secure > being set and adjust the auxv section of the stack. This fixes an > assertion when running ld.so directly with glibc.rtld.enable_secure set. > > elf/rtld.c:1324 assert (auxv == sp + 1); > --- > Changes to v1: > * Add comment explaining how and why skip_env is adjusted. > > elf/rtld.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/elf/rtld.c b/elf/rtld.c > index ac4bb23652..e9525ea987 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -155,7 +155,7 @@ static void dl_main_state_init (struct dl_main_state *state); > Since all of them start with `LD_' we are a bit smarter while finding > all the entries. */ > extern char **_environ attribute_hidden; > -static void process_envvars (struct dl_main_state *state); > +static int process_envvars (struct dl_main_state *state); > > int _dl_argc attribute_relro attribute_hidden; > char **_dl_argv attribute_relro = NULL; > @@ -1287,7 +1287,7 @@ rtld_setup_main_map (struct link_map *main_map) > _dl_argv and _dl_argc accordingly. Those arguments are removed from > argv here. */ > static void > -_dl_start_args_adjust (int skip_args) > +_dl_start_args_adjust (int skip_args, int skip_env) > { > void **sp = (void **) (_dl_argv - skip_args - 1); > void **p = sp + skip_args; > @@ -1319,7 +1319,7 @@ _dl_start_args_adjust (int skip_args) > while (*p != NULL); > > #ifdef HAVE_AUX_VECTOR > - void **auxv = (void **) GLRO(dl_auxv) - skip_args; > + void **auxv = (void **) GLRO(dl_auxv) - skip_args - skip_env; > GLRO(dl_auxv) = (ElfW(auxv_t) *) auxv; /* Aliasing violation. */ > assert (auxv == sp + 1); > > @@ -1350,6 +1350,7 @@ dl_main (const ElfW(Phdr) *phdr, > unsigned int i; > bool rtld_is_main = false; > void *tcbp = NULL; > + int skip_env = 0; > > struct dl_main_state state; > dl_main_state_init (&state); > @@ -1363,7 +1364,7 @@ dl_main (const ElfW(Phdr) *phdr, > #endif > > /* Process the environment variable which control the behaviour. */ > - process_envvars (&state); > + skip_env = process_envvars (&state); > > #ifndef HAVE_INLINED_SYSCALLS > /* Set up a flag which tells we are just starting. */ > @@ -1628,7 +1629,7 @@ dl_main (const ElfW(Phdr) *phdr, > _dl_argv[0] = argv0; > > /* Adjust arguments for the application entry point. */ > - _dl_start_args_adjust (_dl_argv - orig_argv); > + _dl_start_args_adjust (_dl_argv - orig_argv, skip_env); > } > else > { > @@ -2532,11 +2533,12 @@ a filename can be specified using the LD_DEBUG_OUTPUT environment variable.\n"); > } > } > > -static void > +static int > process_envvars_secure (struct dl_main_state *state) > { > char **runp = _environ; > char *envline; > + int skip_env = 0; > > while ((envline = _dl_next_ld_env_entry (&runp)) != NULL) > { > @@ -2578,6 +2580,14 @@ process_envvars_secure (struct dl_main_state *state) > const char *nextp = UNSECURE_ENVVARS; > do > { > + /* Keep track of the number of environment variables that were set in > + the environment and are unset below. Use getenv() which returns > + non-NULL if the variable is set in the environment. This count is > + needed if we need to adjust the location of the AUX vector on the > + stack when running ld.so directly. */ > + if (getenv (nextp) != NULL) > + skip_env++; > + > unsetenv (nextp); > nextp = strchr (nextp, '\0') + 1; > } > @@ -2590,6 +2600,8 @@ process_envvars_secure (struct dl_main_state *state) > || state->mode != rtld_mode_normal > || state->version_info) > _exit (5); > + > + return skip_env; > } > > static void > @@ -2743,13 +2755,16 @@ process_envvars_default (struct dl_main_state *state) > } > } > > -static void > +static int > process_envvars (struct dl_main_state *state) > { > + int skip_env = 0; > if (__glibc_unlikely (__libc_enable_secure)) > - process_envvars_secure (state); > + skip_env += process_envvars_secure (state); > else > process_envvars_default (state); > + > + return skip_env; > } > > #if HP_TIMING_INLINE > -- > 2.43.2 >
Can you please add a test?
Thanks for your review. I added a testcase to the v3[1] patch that triggers the assert without the patch and does not trigger the assert with the patch. I'm not certain it's a useful test though since it's essentially testing that the assertion isn't triggered. Thanks, Joe [1] https://sourceware.org/pipermail/libc-alpha/2024-April/156034.html On Thu, Apr 11, 2024 at 9:46 AM Andreas Schwab <schwab@suse.de> wrote: > > Can you please add a test? > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different." >
diff --git a/elf/rtld.c b/elf/rtld.c index ac4bb23652..e9525ea987 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -155,7 +155,7 @@ static void dl_main_state_init (struct dl_main_state *state); Since all of them start with `LD_' we are a bit smarter while finding all the entries. */ extern char **_environ attribute_hidden; -static void process_envvars (struct dl_main_state *state); +static int process_envvars (struct dl_main_state *state); int _dl_argc attribute_relro attribute_hidden; char **_dl_argv attribute_relro = NULL; @@ -1287,7 +1287,7 @@ rtld_setup_main_map (struct link_map *main_map) _dl_argv and _dl_argc accordingly. Those arguments are removed from argv here. */ static void -_dl_start_args_adjust (int skip_args) +_dl_start_args_adjust (int skip_args, int skip_env) { void **sp = (void **) (_dl_argv - skip_args - 1); void **p = sp + skip_args; @@ -1319,7 +1319,7 @@ _dl_start_args_adjust (int skip_args) while (*p != NULL); #ifdef HAVE_AUX_VECTOR - void **auxv = (void **) GLRO(dl_auxv) - skip_args; + void **auxv = (void **) GLRO(dl_auxv) - skip_args - skip_env; GLRO(dl_auxv) = (ElfW(auxv_t) *) auxv; /* Aliasing violation. */ assert (auxv == sp + 1); @@ -1350,6 +1350,7 @@ dl_main (const ElfW(Phdr) *phdr, unsigned int i; bool rtld_is_main = false; void *tcbp = NULL; + int skip_env = 0; struct dl_main_state state; dl_main_state_init (&state); @@ -1363,7 +1364,7 @@ dl_main (const ElfW(Phdr) *phdr, #endif /* Process the environment variable which control the behaviour. */ - process_envvars (&state); + skip_env = process_envvars (&state); #ifndef HAVE_INLINED_SYSCALLS /* Set up a flag which tells we are just starting. */ @@ -1628,7 +1629,7 @@ dl_main (const ElfW(Phdr) *phdr, _dl_argv[0] = argv0; /* Adjust arguments for the application entry point. */ - _dl_start_args_adjust (_dl_argv - orig_argv); + _dl_start_args_adjust (_dl_argv - orig_argv, skip_env); } else { @@ -2532,11 +2533,12 @@ a filename can be specified using the LD_DEBUG_OUTPUT environment variable.\n"); } } -static void +static int process_envvars_secure (struct dl_main_state *state) { char **runp = _environ; char *envline; + int skip_env = 0; while ((envline = _dl_next_ld_env_entry (&runp)) != NULL) { @@ -2578,6 +2580,14 @@ process_envvars_secure (struct dl_main_state *state) const char *nextp = UNSECURE_ENVVARS; do { + /* Keep track of the number of environment variables that were set in + the environment and are unset below. Use getenv() which returns + non-NULL if the variable is set in the environment. This count is + needed if we need to adjust the location of the AUX vector on the + stack when running ld.so directly. */ + if (getenv (nextp) != NULL) + skip_env++; + unsetenv (nextp); nextp = strchr (nextp, '\0') + 1; } @@ -2590,6 +2600,8 @@ process_envvars_secure (struct dl_main_state *state) || state->mode != rtld_mode_normal || state->version_info) _exit (5); + + return skip_env; } static void @@ -2743,13 +2755,16 @@ process_envvars_default (struct dl_main_state *state) } } -static void +static int process_envvars (struct dl_main_state *state) { + int skip_env = 0; if (__glibc_unlikely (__libc_enable_secure)) - process_envvars_secure (state); + skip_env += process_envvars_secure (state); else process_envvars_default (state); + + return skip_env; } #if HP_TIMING_INLINE