Date: Wed, 22 Mar 2017 11:02:58 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Ed Schouten <ed@FreeBSD.org> 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: <20170322090258.GR43712@kib.kiev.ua> In-Reply-To: <201703220705.v2M75RHE066483@repo.freebsd.org> References: <201703220705.v2M75RHE066483@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. 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..642516cc797 100644 --- a/sys/amd64/cloudabi64/cloudabi64_sysvec.c +++ b/sys/amd64/cloudabi64/cloudabi64_sysvec.c @@ -214,5 +214,5 @@ Elf64_Brandinfo cloudabi64_brand = { .sysvec = &cloudabi64_elf_sysvec, .flags = BI_CAN_EXEC_DYN, .compat_3_brand = "CloudABI", - .interp_path = "/nonexistent", + .flags = BI_BRAND_NOTE_ONLY_STATIC, }; 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..af23ffda9a0 100644 --- a/sys/arm64/cloudabi64/cloudabi64_sysvec.c +++ b/sys/arm64/cloudabi64/cloudabi64_sysvec.c @@ -183,5 +183,5 @@ Elf64_Brandinfo cloudabi64_brand = { .sysvec = &cloudabi64_elf_sysvec, .flags = BI_CAN_EXEC_DYN, .compat_3_brand = "CloudABI", - .interp_path = "/nonexistent", + .flags = BI_BRAND_NOTE_ONLY_STATIC, }; 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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170322090258.GR43712>