From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from iguana.tulip.relay.mailchannels.net (iguana.tulip.relay.mailchannels.net [23.83.218.253]) by sourceware.org (Postfix) with ESMTPS id CB720384AB58 for ; Fri, 3 May 2024 15:30:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CB720384AB58 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=sourceware.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=sourceware.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org CB720384AB58 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=23.83.218.253 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1714750210; cv=pass; b=LyDHvwvD5IsvOffJJefdY3ojyRf96S5voYV9HZwbeSckg8QlSOINaB2alBov4padw9e1SY58en6IXU+dmMeF5R55C7c/Tu+mk3JzMvKjdIt524U0Hhp7SHrLe9SOZT8jz8BAHWc9tF2JS4vKplWU3gk+nwthEi3TLDo/8FdIxrs= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1714750210; c=relaxed/simple; bh=V3sbWYVQDuX6PwRkLw661k35AI+eS4A3DIPBr/2X+90=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=hWIaoTGq4XszkFu/t8sL9VN8wC0di+xltF/qfZy8HEmp/2m9yd0da4yAMUN9X7khIHH7GUBKNGdlfm5C+GtpVqkB8Ywd366MhIDiw+UMTQt7mSou3DO7yWKgfnsDMYkyHR+F6azp1aTjFO8w1ir3fv/2J02h+nby2qaBrYgLQ1Y= ARC-Authentication-Results: i=2; server2.sourceware.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id E0CF52C29BF; Fri, 3 May 2024 15:30:05 +0000 (UTC) Received: from pdx1-sub0-mail-a264.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 3A6BF2C1B13; Fri, 3 May 2024 15:30:04 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1714750204; a=rsa-sha256; cv=none; b=5d0yFVnpC8+tvLXJa5F2oT5A7dAw3b8+prJUzrM1UudX6Ny02zM4winQ02dM4LA6k+8THf 8G8zf9Wg/BdMtsBKOZ4qi/FtyMdMRGfPsCUgCUok1GrArbJH9TV6/M8WRlRwjmaJUAxlXt v+zg0cUMK6IG4dp1sMKsBZmgKrDLdEW18fOtD7fYtszgbhcYEkuJX0nUm2HbcDWK0h2sgE zH+atjPHB5lYoOcv4JN8l1QvUrq4zThVK6T5rCTGscDhdRlP2YHckXcUdUejxF1Dn/XfDP TYKrmpKWkFLe9iHSI+7bSzRNfAerkIY887totho3kFmdZtGtwubd03hLbs1F2w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1714750204; 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=+bZYG24LmWiQMAgzxgJOE3Ik49D5av+F8+hAa9pRWCs=; b=XKgwnDpGaxqifDmH5Y4b7kgIcA8+oSsxPtUPqBR2pgm6UFqm/9SOxhvSrO4zrX6Ubaj34v BxemAtaC+Y4h/LWVAenIj8juFCvrrMklIjzi/vnaaN6pldUtymlrZU65s06sYGDlzRxYtU dF5INQ2fMDt8stb8qS1NbVN3m9sJMwVoGuNw+KSim7Z0qYgHI5ODW3p40w+WEAZTlusYL8 hdOtE7BnQ8FxgRmXk1ldZUM9HmJ84fEUK7b2kjusKC1aHkq4G63MXibjTPri9mwzlBITpC KQizsZVNXyxOGW9JHFPjHa1FkLPi7QK5eNPemKocvtHh/YUFh73Lx0TXN26k/A== ARC-Authentication-Results: i=1; rspamd-5c97bbd7c5-5ktqd; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@sourceware.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Slimy-Wipe: 4275c89539ceded8_1714750204615_4282156484 X-MC-Loop-Signature: 1714750204615:2381706075 X-MC-Ingress-Time: 1714750204615 Received: from pdx1-sub0-mail-a264.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.106.71.32 (trex/6.9.2); Fri, 03 May 2024 15:30:04 +0000 Received: from [192.168.251.119] (unknown [24.114.79.131]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a264.dreamhost.com (Postfix) with ESMTPSA id 4VWF9R1jBLz2R; Fri, 3 May 2024 08:30:03 -0700 (PDT) Message-ID: <1b3beea7-a693-4489-88a2-23eb2a524a2f@sourceware.org> Date: Fri, 3 May 2024 11:30:00 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables To: Adhemerval Zanella , libc-alpha@sourceware.org Cc: Joe Simmons-Talbott References: <20240502163716.1107975-1-adhemerval.zanella@linaro.org> <20240502163716.1107975-5-adhemerval.zanella@linaro.org> Content-Language: en-US From: Siddhesh Poyarekar In-Reply-To: <20240502163716.1107975-5-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-1170.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_SHORT,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_SOFTFAIL,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 2024-05-02 12:35, Adhemerval Zanella wrote: > Tunable with environment variables aliases are also ignored if > glibc.rtld.enable_secure is enabled. The tunable parsing is also > optimized a bit, where the loop that checks each environment variable > only checks for the tunables with aliases instead of all tables. > > Checked on aarch64-linux-gnu and x86_64-linux-gnu. > --- > elf/dl-tunables.c | 36 ++++++--- > elf/tst-tunables-enable_secure.c | 133 +++++++++++++++++++++++++++---- > scripts/gen-tunables.awk | 16 +++- > 3 files changed, 161 insertions(+), 24 deletions(-) > > diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c > index 63cf8c7ab5..e87d0628b2 100644 > --- a/elf/dl-tunables.c > +++ b/elf/dl-tunables.c > @@ -300,6 +300,9 @@ __tunables_init (char **envp) > if (__libc_enable_secure) > return; > > + enum { tunable_num_env_alias = array_length (tunable_env_alias_list) }; > + struct tunable_toset_t tunables_env_alias[tunable_num_env_alias] = { 0 }; > + > while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL) > { > /* The environment variable is allocated on the stack by the kernel, so > @@ -311,29 +314,44 @@ __tunables_init (char **envp) > continue; > } > > - for (int i = 0; i < tunables_list_size; i++) > + for (int i = 0; i < tunable_num_env_alias; i++) > { > - tunable_t *cur = &tunable_list[i]; > + tunable_t *cur = &tunable_list[tunable_env_alias_list[i]]; > + const char *name = cur->env_alias; > > - /* Skip over tunables that have either been set already or should be > - skipped. */ > - if (cur->initialized || cur->env_alias[0] == '\0') > + if (name[0] == '\0') > continue; You don't skip over initialized ones here because you're going to have to check for that later anyway, in case GLIBC_TUNABLES came in after any of the aliases. OK. > > - const char *name = cur->env_alias; > - > - /* We have a match. Initialize and move on to the next line. */ > if (tunable_is_name (name, envname)) > { > size_t envvallen = 0; > /* The environment variable is always null-terminated. */ > for (const char *p = envval; *p != '\0'; p++, envvallen++); > > - tunable_initialize (cur, envval, envvallen); > + tunables_env_alias[i] = > + (struct tunable_toset_t) { cur, envval, envvallen }; > break; > } > } > } > + > + /* Check if glibc.rtld.enable_secure was set and skip over the environment > + variables aliases. */ > + if (__libc_enable_secure) > + return; > + > + for (int i = 0; i < tunable_num_env_alias; i++) > + { > + /* Skip over tunables that have either been set. */ either been set or already initialized? > + if (tunables_env_alias[i].t == NULL > + || tunables_env_alias[i].t->initialized) > + continue; > + > + if (!tunable_initialize (tunables_env_alias[i].t, > + tunables_env_alias[i].value, > + tunables_env_alias[i].len)) > + parse_tunable_print_error (&tunables_env_alias[i]); > + } > } > > void > diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c > index f5db1c84e9..a0da0fd179 100644 > --- a/elf/tst-tunables-enable_secure.c > +++ b/elf/tst-tunables-enable_secure.c > @@ -17,6 +17,7 @@ > . */ > > #include > +#define TUNABLES_INTERNAL 1 OK, I noticed this in patch 1 but it didn't strike me then; this macro will result in a new array tunable_list being initialized in the binary. The tunable references themselves should go via __tunable_get_val and hence refer to the structure in ld.so, so this array only serves as a copy to get the total size. I suppose it's not much for a test case, but do you think it's worth adding a comment here, and also in tst-tunables.c? That way it could be easier to weed out any unforeseen problems with these tests in future. > #include > #include > #include > @@ -34,6 +35,8 @@ static int restart; > static const struct test_t > { > const char *env; > + const char *extraenv; > + bool check_multiple; > int32_t expected_malloc_check; > int32_t expected_enable_secure; > } tests[] = > @@ -41,39 +44,124 @@ static const struct test_t > /* Expected tunable format. */ > /* Tunables should be ignored if enable_secure is set. */ > { > - "glibc.malloc.check=2:glibc.rtld.enable_secure=1", > + "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1", > + NULL, > + false, > 0, > 1, > }, > /* Tunables should be ignored if enable_secure is set. */ > { > - "glibc.rtld.enable_secure=1:glibc.malloc.check=2", > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2", > + NULL, > + false, > 0, > 1, > }, > /* Tunables should be set if enable_secure is unset. */ > { > - "glibc.rtld.enable_secure=0:glibc.malloc.check=2", > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2", Same question as in v1, should we issue a warning for enable_secure=0? We really should never be able to set it to anything other than 1. Which also means that a bunch of these enable_secure=0 tests are probably redundant. > + NULL, > + false, > 2, > 0, > }, > + /* Tunables should be ignored if enable_secure is set. */ > + { > + "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1", > + "MALLOC_CHECK_=2", > + false, > + 0, > + 1, > + }, > + /* Same as before, but with enviroment alias prior GLIBC_TUNABLES. */ > + { > + "MALLOC_CHECK_=2", > + "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1", > + false, > + 0, > + 1, > + }, > + /* Tunables should be ignored if enable_secure is set. */ > + { > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2", > + "MALLOC_CHECK_=2", > + false, > + 0, > + 1, > + }, > + { > + "MALLOC_CHECK_=2", > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2", > + false, > + 0, > + 1, > + }, > + /* Tunables should be set if enable_secure is unset. */ > + { > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2", > + /* Tunable have precedence over the environment variable. */ > + "MALLOC_CHECK_=1", > + false, > + 2, > + 0, > + }, > + { > + "MALLOC_CHECK_=1", > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2", > + /* Tunable have precedence over the environment variable. */ > + false, > + 2, > + 0, > + }, > + /* Tunables should be set if enable_secure is unset. */ > + { > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0", > + /* Tunable have precedence over the environment variable. */ > + "MALLOC_CHECK_=1", > + false, > + 1, > + 0, > + }, > + /* Tunables should be set if enable_secure is unset. */ > + { > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0", > + /* Tunable have precedence over the environment variable. */ > + "MALLOC_CHECK_=1", > + false, > + 1, > + 0, > + }, > + /* Check with tunables environment variable alias set multiple times. */ > + { > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2", > + "MALLOC_CHECK_=2", > + true, > + 0, > + 1, > + }, > + /* Tunables should be set if enable_secure is unset. */ > + { > + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0", > + /* Tunable have precedence over the environment variable. */ > + "MALLOC_CHECK_=1", > + true, > + 1, > + 0, > + }, > }; > > static int > handle_restart (int i) > { > if (tests[i].expected_enable_secure == 1) > - { > - TEST_COMPARE (1, __libc_enable_secure); > - } > + TEST_COMPARE (1, __libc_enable_secure); > else > - { > - TEST_COMPARE (tests[i].expected_malloc_check, > - TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL)); > - TEST_COMPARE (tests[i].expected_enable_secure, > - TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t, > - NULL)); > - } > + TEST_COMPARE (tests[i].expected_enable_secure, > + TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t, > + NULL)); > + TEST_COMPARE (tests[i].expected_malloc_check, > + TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL)); > return 0; > } > > @@ -106,14 +194,31 @@ do_test (int argc, char *argv[]) > spargv[i] = NULL; > } > > + enum { tunable_num_env_alias = array_length (tunable_env_alias_list) }; > + > for (int i = 0; i < array_length (tests); i++) > { > snprintf (nteststr, sizeof nteststr, "%d", i); > > printf ("[%d] Spawned test for %s\n", i, tests[i].env); > setenv ("GLIBC_TUNABLES", tests[i].env, 1); > + > + char *envp[2 + tunable_num_env_alias + 1] = > + { > + (char *) tests[i].env, > + (char *) tests[i].extraenv, > + NULL, > + }; > + if (tests[i].check_multiple) > + { > + int j; > + for (j=0; j < tunable_num_env_alias; j++) > + envp[j + 2] = (char *) tests[i].extraenv; > + envp[j + 2] = NULL; > + } > + > struct support_capture_subprocess result > - = support_capture_subprogram (spargv[0], spargv, NULL); > + = support_capture_subprogram (spargv[0], spargv, envp); > support_capture_subprocess_check (&result, "tst-tunables-enable_secure", > 0, sc_allow_stderr); > support_capture_subprocess_free (&result); > diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk > index 9f5336381e..fc3b41376f 100644 > --- a/scripts/gen-tunables.awk > +++ b/scripts/gen-tunables.awk > @@ -156,7 +156,7 @@ END { > 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 = {" > + print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {" Why is this needed? Doesn't array_length ensures at least one "use"? > for (tnm in types) { > split (tnm, indices, SUBSEP); > t = indices[1]; > @@ -168,5 +168,19 @@ END { > default_val[t,n,m], env_alias[t,n,m]); > } > print "};" > + > + # Map of tunable with environment variables aliases used during parsing. */ > + print "\nstatic const tunable_id_t tunable_env_alias_list[] =" > + printf "{\n" > + for (tnm in types) { > + split (tnm, indices, SUBSEP); > + t = indices[1]; > + n = indices[2]; > + m = indices[3]; > + if (env_alias[t,n,m] != "{0}") { > + printf (" TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m); > + } > + } > + printf "};\n" > print "#endif" > }