From owner-svn-src-head@freebsd.org Wed Mar 22 11:50:31 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id CEF31D15584; Wed, 22 Mar 2017 11:50:31 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 76C0D1227; Wed, 22 Mar 2017 11:50:31 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id v2MBoPjT039155 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 22 Mar 2017 13:50:25 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua v2MBoPjT039155 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id v2MBoPu3039154; Wed, 22 Mar 2017 13:50:25 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 22 Mar 2017 13:50:25 +0200 From: Konstantin Belousov To: Ed Schouten Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r315701 - in head/sys: amd64/cloudabi32 amd64/cloudabi64 arm/cloudabi32 arm64/cloudabi64 i386/cloudabi32 Message-ID: <20170322115025.GT43712@kib.kiev.ua> References: <201703220705.v2M75RHE066483@repo.freebsd.org> <20170322090258.GR43712@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170322090258.GR43712@kib.kiev.ua> User-Agent: Mutt/1.8.0 (2017-02-23) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Mar 2017 11:50:31 -0000 On Wed, Mar 22, 2017 at 11:02:58AM +0200, Konstantin Belousov wrote: > On Wed, Mar 22, 2017 at 07:05:27AM +0000, Ed Schouten wrote: > > Author: ed > > Date: Wed Mar 22 07:05:27 2017 > > New Revision: 315701 > > URL: https://svnweb.freebsd.org/changeset/base/315701 > > > > Log: > > Set the interpreter path to /nonexistent. > > > > CloudABI executables are statically linked and don't have an > > interpreter. Setting the interpreter path to NULL used to work > > previously, but r314851 introduced code that checks the string > > unconditionally. Running CloudABI executables now causes a null pointer > > dereference. > You could have just reported that the revision breaks cloudabi. > > > > > Looking at the rest of imgact_elf.c, it seems various other codepaths > > already leaned on the fact that the interpreter path is set. Let's just > > go ahead and pick an obviously incorrect interpreter path to appease > > imgact_elf.c. > > I believe that we should move in the reverse direction, in particular, > best would be to allow brand to specify that only a statically linked > binaries can be handled by it. My reasoning is coming from a desire > to make brand matching as exact as possible, after dealing with the > bugs due to too vague matching. > > Could you test the following ? It supposedly fixes the NULL issue, and > adds the flag marking brands as only allowing static (really PT_INTERP-less) > binaries. Fix for amd64/arm64 compilation. diff --git a/sys/amd64/cloudabi32/cloudabi32_sysvec.c b/sys/amd64/cloudabi32/cloudabi32_sysvec.c index fe93385e4de..eca42873f8f 100644 --- a/sys/amd64/cloudabi32/cloudabi32_sysvec.c +++ b/sys/amd64/cloudabi32/cloudabi32_sysvec.c @@ -228,5 +228,5 @@ Elf32_Brandinfo cloudabi32_brand = { .machine = EM_386, .sysvec = &cloudabi32_elf_sysvec, .compat_3_brand = "CloudABI", - .interp_path = "/nonexistent", + .flags = BI_BRAND_NOTE_ONLY_STATIC, }; diff --git a/sys/amd64/cloudabi64/cloudabi64_sysvec.c b/sys/amd64/cloudabi64/cloudabi64_sysvec.c index 34277f488fc..f953710971d 100644 --- a/sys/amd64/cloudabi64/cloudabi64_sysvec.c +++ b/sys/amd64/cloudabi64/cloudabi64_sysvec.c @@ -212,7 +212,6 @@ Elf64_Brandinfo cloudabi64_brand = { .brand = ELFOSABI_CLOUDABI, .machine = EM_X86_64, .sysvec = &cloudabi64_elf_sysvec, - .flags = BI_CAN_EXEC_DYN, + .flags = BI_CAN_EXEC_DYN | BI_BRAND_NOTE_ONLY_STATIC, .compat_3_brand = "CloudABI", - .interp_path = "/nonexistent", }; diff --git a/sys/arm/cloudabi32/cloudabi32_sysvec.c b/sys/arm/cloudabi32/cloudabi32_sysvec.c index aea4b822ddc..b1130c92bcd 100644 --- a/sys/arm/cloudabi32/cloudabi32_sysvec.c +++ b/sys/arm/cloudabi32/cloudabi32_sysvec.c @@ -190,5 +190,5 @@ Elf32_Brandinfo cloudabi32_brand = { .machine = EM_ARM, .sysvec = &cloudabi32_elf_sysvec, .compat_3_brand = "CloudABI", - .interp_path = "/nonexistent", + .flags = BI_BRAND_NOTE_ONLY_STATIC, }; diff --git a/sys/arm64/cloudabi64/cloudabi64_sysvec.c b/sys/arm64/cloudabi64/cloudabi64_sysvec.c index 66d569b8f32..45e81c05952 100644 --- a/sys/arm64/cloudabi64/cloudabi64_sysvec.c +++ b/sys/arm64/cloudabi64/cloudabi64_sysvec.c @@ -181,7 +181,6 @@ Elf64_Brandinfo cloudabi64_brand = { .brand = ELFOSABI_CLOUDABI, .machine = EM_AARCH64, .sysvec = &cloudabi64_elf_sysvec, - .flags = BI_CAN_EXEC_DYN, + .flags = BI_CAN_EXEC_DYN | BI_BRAND_NOTE_ONLY_STATIC, .compat_3_brand = "CloudABI", - .interp_path = "/nonexistent", }; diff --git a/sys/i386/cloudabi32/cloudabi32_sysvec.c b/sys/i386/cloudabi32/cloudabi32_sysvec.c index 2658c4f9ed6..d03c9e267ab 100644 --- a/sys/i386/cloudabi32/cloudabi32_sysvec.c +++ b/sys/i386/cloudabi32/cloudabi32_sysvec.c @@ -201,5 +201,5 @@ Elf32_Brandinfo cloudabi32_brand = { .machine = EM_386, .sysvec = &cloudabi32_elf_sysvec, .compat_3_brand = "CloudABI", - .interp_path = "/nonexistent", + .flags = BI_BRAND_NOTE_ONLY_STATIC, }; diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c index f33dc4cbeae..3036f814faf 100644 --- a/sys/kern/imgact_elf.c +++ b/sys/kern/imgact_elf.c @@ -273,6 +273,9 @@ __elfN(get_brandinfo)(struct image_params *imgp, const char *interp, bi = elf_brand_list[i]; if (bi == NULL) continue; + if (interp != NULL && + (bi->flags & BI_BRAND_NOTE_ONLY_STATIC) != 0) + continue; if (hdr->e_machine == bi->machine && (bi->flags & (BI_BRAND_NOTE|BI_BRAND_NOTE_MANDATORY)) != 0) { ret = __elfN(check_note)(imgp, bi->brand_note, osrel); @@ -305,7 +308,9 @@ __elfN(get_brandinfo)(struct image_params *imgp, const char *interp, /* If the executable has a brand, search for it in the brand list. */ for (i = 0; i < MAX_BRANDS; i++) { bi = elf_brand_list[i]; - if (bi == NULL || bi->flags & BI_BRAND_NOTE_MANDATORY) + if (bi == NULL || (bi->flags & BI_BRAND_NOTE_MANDATORY) != 0 || + (interp != NULL && (bi->flags & + BI_BRAND_NOTE_ONLY_STATIC) != 0)) continue; if (hdr->e_machine == bi->machine && (hdr->e_ident[EI_OSABI] == bi->brand || @@ -318,7 +323,11 @@ __elfN(get_brandinfo)(struct image_params *imgp, const char *interp, * Again, prefer strictly matching * interpreter path. */ - if (strlen(bi->interp_path) + 1 == + if (interp_name_len == 0 && + bi->interp_path == NULL) + return (bi); + if (bi->interp_path != NULL && + strlen(bi->interp_path) + 1 == interp_name_len && strncmp(interp, bi->interp_path, interp_name_len) == 0) return (bi); @@ -347,7 +356,8 @@ __elfN(get_brandinfo)(struct image_params *imgp, const char *interp, if (interp != NULL) { for (i = 0; i < MAX_BRANDS; i++) { bi = elf_brand_list[i]; - if (bi == NULL || bi->flags & BI_BRAND_NOTE_MANDATORY) + if (bi == NULL || (bi->flags & (BI_BRAND_NOTE_MANDATORY | + BI_BRAND_NOTE_ONLY_STATIC)) != 0) continue; if (hdr->e_machine == bi->machine && /* ELF image p_filesz includes terminating zero */ @@ -361,7 +371,9 @@ __elfN(get_brandinfo)(struct image_params *imgp, const char *interp, /* Lacking a recognized interpreter, try the default brand */ for (i = 0; i < MAX_BRANDS; i++) { bi = elf_brand_list[i]; - if (bi == NULL || bi->flags & BI_BRAND_NOTE_MANDATORY) + if (bi == NULL || (bi->flags & BI_BRAND_NOTE_MANDATORY) != 0 || + (interp != NULL && (bi->flags & + BI_BRAND_NOTE_ONLY_STATIC) != 0)) continue; if (hdr->e_machine == bi->machine && __elfN(fallback_brand) == bi->brand) diff --git a/sys/sys/imgact_elf.h b/sys/sys/imgact_elf.h index beb5f96fc03..83570ae2399 100644 --- a/sys/sys/imgact_elf.h +++ b/sys/sys/imgact_elf.h @@ -80,6 +80,7 @@ typedef struct { #define BI_CAN_EXEC_DYN 0x0001 #define BI_BRAND_NOTE 0x0002 /* May have note.ABI-tag section. */ #define BI_BRAND_NOTE_MANDATORY 0x0004 /* Must have note.ABI-tag section. */ +#define BI_BRAND_NOTE_ONLY_STATIC 0x0008 } __ElfN(Brandinfo); __ElfType(Auxargs);