From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x29.google.com (mail-oa1-x29.google.com [IPv6:2001:4860:4864:20::29]) by sourceware.org (Postfix) with ESMTPS id BED9D3858D35 for ; Tue, 16 Apr 2024 16:22:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BED9D3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org BED9D3858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:4860:4864:20::29 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713284541; cv=none; b=r9PpHm8Ac4xICPjEMNHwaOQvs6ijP+huKp3d70N+EyDmo+XXH5FT8WXZizTR2taUjAAcHm3IHchVC63v6u1G7nIuw0bzYLtN0Ij8EtHSEpvr8FHCCufDSyV/I0ujNMHx9Suzd8S36wkL1fpJcV+IxTTarKzOlhA8r46M6EYZAP0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713284541; c=relaxed/simple; bh=J5Xa1QYWnPgfm7Oze8MVrfjIblXgsnufBAS/kN1WsoA=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Ss7dceqnYShXYl/w6CJ35jFkpYx7g/ZMI7zkYDhOtfDTMQAPKXkVWGtLXiyBUZyw//KBvgTX8AadYMMy6kiA93muuEfJyvEMSqVfJwHVPn03wqJsMOD/mLCQ12T7bLBGHVRsyKJMVCn7wfdsb/EgXCDXGtV1cMuwWRQe4QxPudM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-oa1-x29.google.com with SMTP id 586e51a60fabf-22fa7e4b0beso2943555fac.1 for ; Tue, 16 Apr 2024 09:22:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1713284538; x=1713889338; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=ThypSRYRS8XvxEBTunOOC6OBKyKByB23MJejgI8CvJs=; b=Rzievr7v/KWjbVPAlvM9sDa10el5QFIdnyjg6Vzb6eI0EQov5/Bp0xqpnMWt2sYz1n XM2B2J021VLVdaR9ax2PuBzHHpy2wm8IzBbH2RvvGDmmcFrtW0HN4Wao0YFFq0qAbH41 v7S41mT0KEGNCa5efF2Rbhi480e6Z4Q27/K+H9joyt7AEuIrOCEnjTo9fXpOE5ntSQL3 B9pZkw90Z1TzTDtg/JLsGtnAcjFcOXLxPMYbOC6d6P48L0j4Eq6s5tyhNcFgW71R96au /HV++Jvc3EaGF+KdGLmWuxmd2HoP485xxsakJaJcy7NZ7b3smApcGVHTmQflh4WknPv5 uZsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713284538; x=1713889338; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ThypSRYRS8XvxEBTunOOC6OBKyKByB23MJejgI8CvJs=; b=ktzskJgE4ndmM6nHnaKqMY11KXTjntWJDXP+2AeRX07uk6b/cAc9fqnvmOx0m7vHBy WhCgV4X4713uo5tpnC7ZMI5dOSzvTfXYo3z0+Hu3L3EdBIBktPy3bFTY75z0G3QQnHK9 V7qi8LQ/cepHyhYILUFpmwAKGtBD05OHAnyRDqMTETXYivkbYmAjqZ0ioX4Hmahhzd6W QHI1DhDSQHzy6L8fFhoC5YoAHtRAH9XYT2Ghy7mv8U9f5DKNFZrHdPTccYpWNda7Q7wn BLYQNa9W8vtaD0EhGTadDEtLMft5LSwAPF1816+TdMsFvxlVc2yzB5WXIUBMNpEt0Xgm qRzw== X-Forwarded-Encrypted: i=1; AJvYcCWwdcUjhMSANn4Hsj9fqEULYkKYLTG3pbu4rREWurTP5X7Z9EJ9B0WV4BYDct8aTkBbgjsnJvIU7neZl8NJBYUYA08ZeFFnIim+ X-Gm-Message-State: AOJu0YztyjnTokZqfSvSLO3EeyTU30grdaOM24M5TuWJdalAIIvRhuPn QvKFWqcwd/42UNNj+Cix5S+7W11F7aOPWE+G6X7QnynGi3ZaHL4S3TWuaS8XDnEoJVDTpVl3M/1 h X-Google-Smtp-Source: AGHT+IGpGATEivEf6WCZVGbsMHht8yb7exLw00SE7A/sVKjcGfrHx9x4UL00skfmW8AwqJH+07yNFQ== X-Received: by 2002:a05:6871:31b0:b0:22e:ca72:faad with SMTP id lv48-20020a05687131b000b0022eca72faadmr9412673oac.43.1713284538056; Tue, 16 Apr 2024 09:22:18 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c0:d8e5:34bc:e1c9:2b45:c06a? ([2804:1b3:a7c0:d8e5:34bc:e1c9:2b45:c06a]) by smtp.gmail.com with ESMTPSA id k187-20020a636fc4000000b005d6a0b2efb3sm8993941pgc.21.2024.04.16.09.22.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 16 Apr 2024 09:22:17 -0700 (PDT) Message-ID: Date: Tue, 16 Apr 2024 13:22:14 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4] elf/rtld: Count skipped environment variables for enable_secure To: Joe Simmons-Talbott , libc-alpha@sourceware.org Cc: Andreas Schwab , Carlos O'Donell References: <20240412152126.663092-1-josimmon@redhat.com> Content-Language: en-US From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <20240412152126.663092-1-josimmon@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 12/04/24 12:21, Joe Simmons-Talbott 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. > Add a testcase that ensures the assert is not hit. > > elf/rtld.c:1324 assert (auxv == sp + 1); LGTM, thanks. Reviewed-by: Adhemerval Zanella > --- > Changes to v3: > * Run the testcase with ld.so explicitly. > > Changes to v2: > * Add testcase that triggers the assertion before this patch and does > not after the patch. > > Changes to v1: > * Add comment explaining how and why skip_env is adjusted. > > elf/Makefile | 10 +++++++++ > elf/rtld.c | 31 +++++++++++++++++++------- > elf/tst-tunables-enable_secure-env.c | 33 ++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+), 8 deletions(-) > create mode 100644 elf/tst-tunables-enable_secure-env.c > > diff --git a/elf/Makefile b/elf/Makefile > index 6dad11bcfb..ea8a078f3e 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -315,6 +315,7 @@ tests := \ > tst-leaks1 \ > tst-stringtable \ > tst-tls9 \ > + tst-tunables-enable_secure-env \ > # tests > > tests-internal := \ > @@ -1189,6 +1190,7 @@ tests-special += \ > $(objpfx)tst-trace3.out \ > $(objpfx)tst-trace4.out \ > $(objpfx)tst-trace5.out \ > + $(objpfx)tst-tunables-enable_secure-env.out \ > $(objpfx)tst-unused-dep-cmp.out \ > $(objpfx)tst-unused-dep.out \ > # tests-special > @@ -2216,6 +2218,14 @@ $(objpfx)tst-unused-dep-cmp.out: $(objpfx)tst-unused-dep.out > cmp $< /dev/null > $@; \ > $(evaluate-test) > > +$(objpfx)tst-tunables-enable_secure-env.out: $(objpfx)tst-tunables-enable_secure-env > + $(test-wrapper-env) \ > + GLIBC_TUNABLES=glibc.rtld.enable_secure=1 \ > + $(rtld-prefix) \ > + $< > $@; \ > + $(evaluate-test) > + > + > $(objpfx)tst-audit11.out: $(objpfx)tst-auditmod11.so $(objpfx)tst-audit11mod1.so > tst-audit11-ENV = LD_AUDIT=$(objpfx)tst-auditmod11.so > $(objpfx)tst-audit11mod1.so: $(objpfx)tst-audit11mod2.so > diff --git a/elf/rtld.c b/elf/rtld.c > index 18d73f19c6..d116a436f5 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; > @@ -1289,7 +1289,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; > @@ -1321,7 +1321,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); > > @@ -1352,6 +1352,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); > @@ -1365,7 +1366,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. */ > @@ -1630,7 +1631,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 > { > @@ -2534,11 +2535,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) > { > @@ -2580,6 +2582,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; > } > @@ -2592,6 +2602,8 @@ process_envvars_secure (struct dl_main_state *state) > || state->mode != rtld_mode_normal > || state->version_info) > _exit (5); > + > + return skip_env; > } > > static void > @@ -2745,13 +2757,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 > diff --git a/elf/tst-tunables-enable_secure-env.c b/elf/tst-tunables-enable_secure-env.c > new file mode 100644 > index 0000000000..24e846f299 > --- /dev/null > +++ b/elf/tst-tunables-enable_secure-env.c > @@ -0,0 +1,33 @@ > +/* Check enable_secure tunable handles removed ENV variables without > + assertions. > + Copyright (C) 2024 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > +#include > + > +static int > +do_test (int argc, char *argv[]) > +{ > + /* Ensure that no assertions are hit when a dynamically linked application > + runs. This test requires that GLIBC_TUNABLES=glibc.rtld.enable_secure=1 > + is set. */ > + return 0; > +} > + > +#define TEST_FUNCTION_ARGV do_test > +#include