From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 630443858D38 for ; Fri, 12 Apr 2024 15:24:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 630443858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 630443858D38 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712935450; cv=none; b=OUtHRsI8VI1fuOCw+Nh8XA8N+bI8fLZ0KdiQqdh+KP/kI+0yru+kHC5fxXNJvitccyB6lA/RlIZGrE1G+/jgBsK/XAHqBe764iKDfAtoC9CzSEOWlJH8wPQrL8E2/f8O5CHoEERFfnpbuprSx02DFDYzjxUFb8Xzu6UBvRb+U+M= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712935450; c=relaxed/simple; bh=uPLvxPpNHGv+fUREyDl2Z/wC7j6j/mAsyyJL5MWYikI=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=J124fUNk2sjmTvLXSMNSvnoRcE1H0ANOrJQdHdzLHPWzySPNrA/3ZIr8a8fmqzSEtioWrGpBnZRbCW9KkW4X1vXRW3m0zDpitD5CSz3kOv8EN+3BAnKVhhz0YycOP6C7E9r4MgSYIXYZB9cQrPN0oDFne9VEJVACgPYrJsu/yjA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1712935448; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=EivpoH/sv6ywA3q0fjk2lkCxolcKVXKEIWX0QE6zZ2Q=; b=LleQJMzZ9CQHXXBdCvfZVNlnKyeM3iD38WBbOlO/WRku+Mm2O8fUWNMqW5LVYBKo8hvysw dX4G9TBXB/7cKWcH2P8TkkHKwzs2sJOnBNlIv7ttlobnyCfKItVIGGmWgW4rtiHftV+1x4 kRmYOG3TKLqF2Z/RSZ64nZWsWcgkROg= Received: from mail-lf1-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-519-jDLRzWwdOiimPkj9roOvcQ-1; Fri, 12 Apr 2024 11:24:06 -0400 X-MC-Unique: jDLRzWwdOiimPkj9roOvcQ-1 Received: by mail-lf1-f72.google.com with SMTP id 2adb3069b0e04-516c8697daaso859747e87.2 for ; Fri, 12 Apr 2024 08:24:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712935445; x=1713540245; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=EivpoH/sv6ywA3q0fjk2lkCxolcKVXKEIWX0QE6zZ2Q=; b=oEqfy5gSUZ+ONqzKfch8VvxB6i40TYf3dIKVGEduZgCS9MKil7scrnUK7PPg4x+rRO 6rI+5z8idGGiuJvkT+Gg/aXVmK1nOUoDZkDGtHZ7IBWZBG1CcjStPNkCc5XpFFsUxgDy s5C1nKqiafqesXJnXVLwVZ6SpzsF087tLK9wFYZ3ZQYi1sVw3zqLHIpnNyA/BHCx+bs8 6vEig4lhyFI9tt8n4rUa+GmYX96lgJn3LL+EbfXA9zARI+53tJrlRYC223FDPnFrYGaK aoqb36kUxseCrWgiubrc80VS8aKNvYN7Wy2xL91Xy6hUTSsWmZRhTnjXVGcLMF3FlxGQ 1X2Q== X-Gm-Message-State: AOJu0Yzpr5rTAv0v+sl42cIgQbF0tHtD41tenHWYCrVLSKytJKV6tRFy kXyvsTZtD/ju86efcqNfe6BIqFkzQfFDRBP2pKWF5/6j8fCNbSLB+W9NZ/QIQwIzKw0K3i16xdQ KCE4YEeOo3WzpkeEZvkSZTfkqToNubgqDJY4cMWjr3xGgDSiSYMD/B48DuEN18OgzWG0ub2VeWK MxgE8+VvHqoz6Rf+Plz97TWYZ25PLwATkE X-Received: by 2002:a05:6512:504:b0:516:bead:a1b6 with SMTP id o4-20020a056512050400b00516beada1b6mr1799851lfb.5.1712935445042; Fri, 12 Apr 2024 08:24:05 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEOYJBEnR4YxEgxvJdkAMe0AJc6rFGG94nVrhAoP7SFoamsxs99wwH+JrMd+BTvo2GfAmVbQ4cqEr5HH1ERB8I= X-Received: by 2002:a05:6512:504:b0:516:bead:a1b6 with SMTP id o4-20020a056512050400b00516beada1b6mr1799830lfb.5.1712935444555; Fri, 12 Apr 2024 08:24:04 -0700 (PDT) MIME-Version: 1.0 References: <20240411165342.3511107-1-josimmon@redhat.com> <2c2922df-4900-4efb-b317-6d6760c5a61c@linaro.org> In-Reply-To: <2c2922df-4900-4efb-b317-6d6760c5a61c@linaro.org> From: Joe Simmons-Talbott Date: Fri, 12 Apr 2024 11:23:48 -0400 Message-ID: Subject: Re: [PATCH v3] elf/rtld: Count skipped environment variables for enable_secure To: Adhemerval Zanella Netto Cc: libc-alpha@sourceware.org, Andreas Schwab , "Carlos O'Donell" X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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 Fri, Apr 12, 2024 at 10:14=E2=80=AFAM Adhemerval Zanella Netto wrote: > > > > On 11/04/24 13:53, Joe Simmons-Talbott wrote: > > When using the glibc.rtld.enable_secure tunable we need to keep track o= f > > 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 =3D=3D sp + 1); > > --- > > 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 | 3 +++ > > elf/rtld.c | 31 +++++++++++++++++++------- > > elf/tst-tunables-enable_secure-env.c | 33 ++++++++++++++++++++++++++++ > > 3 files changed, 59 insertions(+), 8 deletions(-) > > create mode 100644 elf/tst-tunables-enable_secure-env.c > > > > diff --git a/elf/Makefile b/elf/Makefile > > index 6dad11bcfb..5c35db1799 100644 > > --- a/elf/Makefile > > +++ b/elf/Makefile > > @@ -315,6 +315,7 @@ tests :=3D \ > > tst-leaks1 \ > > tst-stringtable \ > > tst-tls9 \ > > + tst-tunables-enable_secure-env \ > > # tests > > > > tests-internal :=3D \ > > @@ -336,6 +337,8 @@ tst-tls9-static-ENV =3D $(static-dlopen-environment= ) > > tst-single_threaded-static-dlopen-ENV =3D $(static-dlopen-environment) > > tst-rootdir-ENV =3D LD_LIBRARY_PATH=3D/ > > > > +tst-tunables-enable_secure-env-ENV =3D GLIBC_TUNABLES=3Dglibc.rtld.ena= ble_secure=3D1 > > + > > I think since the idea is to check for running ld.so directly, you will n= eed to > explicit issue the loader: > > diff --git a/elf/Makefile b/elf/Makefile > index 5c35db1799..1598fbd6d7 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -337,8 +337,6 @@ tst-tls9-static-ENV =3D $(static-dlopen-environment) > tst-single_threaded-static-dlopen-ENV =3D $(static-dlopen-environment) > tst-rootdir-ENV =3D LD_LIBRARY_PATH=3D/ > > -tst-tunables-enable_secure-env-ENV =3D GLIBC_TUNABLES=3Dglibc.rtld.enabl= e_secure=3D1 > - > tests +=3D \ > argv0test \ > constload1 \ > @@ -1192,6 +1190,7 @@ tests-special +=3D \ > $(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 > @@ -2219,6 +2218,13 @@ $(objpfx)tst-unused-dep-cmp.out: $(objpfx)tst-unus= ed-dep.out > cmp $< /dev/null > $@; \ > $(evaluate-test) > > +$(objpfx)tst-tunables-enable_secure-env.out: $(objpfx)tst-tunables-enabl= e_secure-env > + $(test-wrapper-env) \ > + GLIBC_TUNABLES=3Dglibc.rtld.enable_secure=3D1 \ > + $(rtld-prefix) \ > + $< > $@; \ > + $(evaluate-test) > + > $(objpfx)tst-audit11.out: $(objpfx)tst-auditmod11.so $(objpfx)tst-audit1= 1mod1.so > tst-audit11-ENV =3D LD_AUDIT=3D$(objpfx)tst-auditmod11.so > $(objpfx)tst-audit11mod1.so: $(objpfx)tst-audit11mod2.so > > Otherwise this won't check the bug with --enable-hardcoded-path-in-tests. > > The rest looks ok. Thanks for the review. I've posted a v4 [1] with this change. Thanks, Joe [1] https://sourceware.org/pipermail/libc-alpha/2024-April/156040.html > > > tests +=3D \ > > argv0test \ > > constload1 \ > > 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_stat= e *state); > > Since all of them start with `LD_' we are a bit smarter while findi= ng > > 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 =3D NULL; > > @@ -1289,7 +1289,7 @@ rtld_setup_main_map (struct link_map *main_map) > > _dl_argv and _dl_argc accordingly. Those arguments are removed fro= m > > argv here. */ > > static void > > -_dl_start_args_adjust (int skip_args) > > +_dl_start_args_adjust (int skip_args, int skip_env) > > { > > void **sp =3D (void **) (_dl_argv - skip_args - 1); > > void **p =3D sp + skip_args; > > @@ -1321,7 +1321,7 @@ _dl_start_args_adjust (int skip_args) > > while (*p !=3D NULL); > > > > #ifdef HAVE_AUX_VECTOR > > - void **auxv =3D (void **) GLRO(dl_auxv) - skip_args; > > + void **auxv =3D (void **) GLRO(dl_auxv) - skip_args - skip_env; > > GLRO(dl_auxv) =3D (ElfW(auxv_t) *) auxv; /* Aliasing violation. */ > > assert (auxv =3D=3D sp + 1); > > > > @@ -1352,6 +1352,7 @@ dl_main (const ElfW(Phdr) *phdr, > > unsigned int i; > > bool rtld_is_main =3D false; > > void *tcbp =3D NULL; > > + int skip_env =3D 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 =3D 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] =3D 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 =3D _environ; > > char *envline; > > + int skip_env =3D 0; > > > > while ((envline =3D _dl_next_ld_env_entry (&runp)) !=3D NULL) > > { > > @@ -2580,6 +2582,14 @@ process_envvars_secure (struct dl_main_state *st= ate) > > const char *nextp =3D UNSECURE_ENVVARS; > > do > > { > > + /* Keep track of the number of environment variables that were s= et in > > + the environment and are unset below. Use getenv() which retu= rns > > + 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 th= e > > + stack when running ld.so directly. */ > > + if (getenv (nextp) !=3D NULL) > > + skip_env++; > > + > > unsetenv (nextp); > > nextp =3D strchr (nextp, '\0') + 1; > > } > > @@ -2592,6 +2602,8 @@ process_envvars_secure (struct dl_main_state *sta= te) > > || state->mode !=3D 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 =3D 0; > > if (__glibc_unlikely (__libc_enable_secure)) > > - process_envvars_secure (state); > > + skip_env +=3D 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-en= able_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 appli= cation > > + runs. This test requires that GLIBC_TUNABLES=3Dglibc.rtld.enable= _secure=3D1 > > + is set. */ > > + return 0; > > +} > > + > > +#define TEST_FUNCTION_ARGV do_test > > +#include > --=20 Joe Simmons-Talbott