From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) by sourceware.org (Postfix) with ESMTPS id D4D803858006 for ; Mon, 20 Dec 2021 12:50:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D4D803858006 Received: by mail-qt1-x833.google.com with SMTP id n15so9643483qta.0 for ; Mon, 20 Dec 2021 04:50:34 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=4x7SF2v7nxriKPQmN56sQC1fe0mo09OApzegVG3AdZU=; b=x5auLyopG0RcqpG9l5hDpQmik57yNuRYcMYRakItxQh2p0Hb3yaXcOrqaKUWNElIzk zsMh/SPfgYnBvOlpHDAYsuWIH0TF2LUE1cBAZA5A/R0s2h/khTYk+mDenZcxTbFJil39 MuF3NOI2fUFFnrj5VTsFmBCA9N3UivM4Fls0vKYVrIgGjE81zQxw6YobeTVjuIGcYxOQ FSeAVApZa5KJRC9YLg7Bv5ddVDW4Ru2c2rC9tkeHrU/W+VeKLnX0qCLTaW7xpJC5xEye bYzjZpIZaUqvVOYve6SkuluodSbzx0qP0i1ZrHO3bA/iXo+DMc0iTbtu6LKnyfvvlwRh 47Pw== X-Gm-Message-State: AOAM532JIDj2KyTwNd2H9wVj/keILK6UBFnKO4cUvgDSWaNUedqqbWcB cZZmzOB76dx9ojIyoHyB/AZhMJV+pamr9w== X-Google-Smtp-Source: ABdhPJy+ihMfkT2OSZ0XVDgMehgEmlJDxrRn5nYgXRamKd0f5kn8o7O4WWIFogDTp82z1N4UG2vI/g== X-Received: by 2002:a05:622a:15cc:: with SMTP id d12mr1926475qty.387.1640004633842; Mon, 20 Dec 2021 04:50:33 -0800 (PST) Received: from ?IPV6:2804:431:c7cb:3b1e:762b:24f5:94b:4e15? ([2804:431:c7cb:3b1e:762b:24f5:94b:4e15]) by smtp.gmail.com with ESMTPSA id x15sm11405854qko.82.2021.12.20.04.50.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Dec 2021 04:50:33 -0800 (PST) Message-ID: <98044a74-6b2a-8b53-aecf-1d4832264e6a@linaro.org> Date: Mon, 20 Dec 2021 09:50:31 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH v6 14/20] elf: Issue audit la_objopen() for vDSO Content-Language: en-US To: Florian Weimer Cc: libc-alpha@sourceware.org, John Mellor-Crummey , Ben Woodard References: <20211115183734.531155-1-adhemerval.zanella@linaro.org> <20211115183734.531155-15-adhemerval.zanella@linaro.org> <878rwhheem.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella In-Reply-To: <878rwhheem.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Dec 2021 12:50:36 -0000 On 18/12/2021 17:00, Florian Weimer wrote: > * Adhemerval Zanella: > >> The vDSO is is listed in the link_map chain, but is never the subject of >> an la_objopen call. A new internal flag __RTLD_VDSO is added that >> acts as __RTLD_OPENEXEC to allocate the required 'struct auditstate' >> extra space for the 'struct link_map'. >> >> The return value from the callback() is currently ignored, since there >> is no PLT call involved by glibc when using the vDSO, neither the vDSO >> are exported directly. > > The usual comment about (). 8-) Ack. > >> diff --git a/elf/dl-object.c b/elf/dl-object.c >> index eb2158a84b..6f26da4310 100644 >> --- a/elf/dl-object.c >> +++ b/elf/dl-object.c >> @@ -59,16 +59,19 @@ _dl_new_object (char *realname, const char *libname, int type, >> { >> #ifdef SHARED >> unsigned int naudit; >> - if (__glibc_unlikely ((mode & __RTLD_OPENEXEC) != 0)) >> + if (__glibc_unlikely ((mode & (__RTLD_OPENEXEC | __RTLD_VDSO)) != 0)) >> { >> - assert (type == lt_executable); >> - assert (nsid == LM_ID_BASE); >> + if (mode & __RTLD_OPENEXEC) >> + { >> + assert (type == lt_executable); >> + assert (nsid == LM_ID_BASE); >> >> - /* Ignore the specified libname for the main executable. It is >> - only known with an explicit loader invocation. */ >> - libname = ""; >> + /* Ignore the specified libname for the main executable. It is >> + only known with an explicit loader invocation. */ >> + libname = ""; >> + } >> >> - /* We create the map for the executable before we know whether >> + /* We create the map for the executable and vDSO before we know whether >> we have auditing libraries and if yes, how many. Assume the >> worst. */ >> naudit = DL_NNS; > > Okay, we call _dl_new_object early for the vDSO, too, so the comment is > appropriate. > >> diff --git a/elf/tst-audit22.c b/elf/tst-audit22.c >> new file mode 100644 >> index 0000000000..f136f25a32 >> --- /dev/null >> +++ b/elf/tst-audit22.c > >> + /* The respawned process should always print the vDSO address (otherwise it >> + will fails as unsupported). However, on some architectures the audit >> + module might see the vDSO with l_addr being 0, meaning a fixed mapping >> + (linux-gate.so). In this case we don't its value against AT_SYSINFO_EHDR >> + one. */ > > “we don't [check] its value” Ack. > >> + uintptr_t vdso_process = 0; >> + bool vdso_audit_found = false; >> + uintptr_t vdso_audit = 0; >> + >> + FILE *out = fmemopen (result.err.buffer, result.err.length, "r"); >> + TEST_VERIFY (out != NULL); >> + char *buffer = NULL; >> + size_t buffer_length = 0; >> + while (xgetline (&buffer, &buffer_length, out)) >> + { >> + if (startswith (buffer, "vdso: ")) >> + vdso_process = parse_address (buffer + strlen ("vdso ")); > > "vdso: " (with colon), but sscanf already skips the leading space. Ack. > >> +#define TEST_FUNCTION_ARGV do_test >> +#include >> diff --git a/elf/tst-auditmod22.c b/elf/tst-auditmod22.c >> new file mode 100644 >> index 0000000000..8e05ce8cbb > >> +static inline bool >> +startswith (const char *str, const char *pre) >> +{ >> + size_t lenpre = strlen (pre); >> + size_t lenstr = strlen (str); >> + return lenstr < lenpre ? false : memcmp (pre, str, lenpre) == 0; >> +} > > lenstr >= lenpre && memcmp (pre, str, lenpre) == 0 Ack. > >> + >> +unsigned int >> +la_version (unsigned int version) >> +{ >> + return LAV_CURRENT; >> +} >> + >> +unsigned int >> +la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t *cookie) >> +{ >> + /* The linux-gate.so is placed at a fixed address, thus l_addr being 0, >> + and it might be the value reported as the AT_SYSINFO_EHDR. */ >> + if (map->l_addr == 0 && startswith (map->l_name, "linux-gate.so")) >> + fprintf (stderr, "vdso found: %p\n", NULL); >> + else if (map->l_addr == getauxval (AT_SYSINFO_EHDR)) >> + fprintf (stderr, "vdso found: %p\n", (void*) map->l_addr); >> + >> + return 0; >> +} > > Would it be possible to look at the program headers to get the minimum > mapped address for the linux-gate.so object? It should be possible, but I only saw it on a ia64 machine (which I don't have access anymore) and reading the kernel source linux-gate.so is provided also for x86, sh, and sparc 32-bits kernels. I am not sure if it worth the trouble. > >> diff --git a/include/dlfcn.h b/include/dlfcn.h >> index a4c283728f..66bcf2dff9 100644 >> --- a/include/dlfcn.h >> +++ b/include/dlfcn.h >> @@ -12,6 +12,7 @@ >> #define __RTLD_AUDIT 0x08000000 >> #define __RTLD_SECURE 0x04000000 /* Apply additional security checks. */ >> #define __RTLD_NOIFUNC 0x02000000 /* Suppress calling ifunc functions. */ >> +#define __RTLD_VDSO 0x01000000 >> >> #define __LM_ID_CALLER -2 > > The __RTLD_VDSO definition is unique, okay. But maybe add a comment, > like “Tell _dl_new_object the object is system-loaded.” Ack. > > Thanks, > Florian >