From: Dave Martin via Libc-alpha <libc-alpha@sourceware.org>
To: Mark Brown <broonie@kernel.org>
Cc: linux-arch@vger.kernel.org, Yu-cheng Yu <yu-cheng.yu@intel.com>,
libc-alpha@sourceware.org, Szabolcs Nagy <szabolcs.nagy@arm.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Jeremy Linton <jeremy.linton@arm.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 1/4] elf: Allow architectures to parse properties on the main executable
Date: Mon, 19 Jul 2021 11:04:39 +0100 [thread overview]
Message-ID: <20210719100427.GT4187@arm.com> (raw)
In-Reply-To: <20210712115259.29547-2-broonie@kernel.org>
On Mon, Jul 12, 2021 at 12:52:56PM +0100, Mark Brown wrote:
> Currently the ELF code only attempts to parse properties on the image
> that will start execution, either the interpreter or for statically linked
> executables the main executable. The expectation is that any property
> handling for the main executable will be done by the interpreter. This is
> a bit inconsistent since we do map the executable and is causing problems
> for the arm64 BTI support when used in conjunction with systemd's use of
> seccomp to implement MemoryDenyWriteExecute which stops the dynamic linker
> adjusting the permissions of executable segments.
>
> Allow architectures to handle properties for both the dynamic linker and
> main executable, adjusting arch_parse_elf_properties() to have an is_interp
Nit: to have has_interp and is_interp flags
> flag as with arch_elf_adjust_prot() and calling it for both the main
> executable and any intepreter.
It would be helpful to note that the user of this code (arm64) is
adapted to ensure that there is no functional change; otherwise the
arm64 changes look a little non-obvious.
This is not a big deal though, and the code looks fine.
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
So, with or without the commit message tweaks:
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> ---
> arch/arm64/include/asm/elf.h | 3 ++-
> fs/binfmt_elf.c | 31 +++++++++++++++++++++++--------
> include/linux/elf.h | 4 +++-
> 3 files changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
> index 8d1c8dcb87fd..a488a1329b16 100644
> --- a/arch/arm64/include/asm/elf.h
> +++ b/arch/arm64/include/asm/elf.h
> @@ -261,6 +261,7 @@ struct arch_elf_state {
>
> static inline int arch_parse_elf_property(u32 type, const void *data,
> size_t datasz, bool compat,
> + bool has_interp, bool is_interp,
> struct arch_elf_state *arch)
> {
> /* No known properties for AArch32 yet */
> @@ -273,7 +274,7 @@ static inline int arch_parse_elf_property(u32 type, const void *data,
> if (datasz != sizeof(*p))
> return -ENOEXEC;
>
> - if (system_supports_bti() &&
> + if (system_supports_bti() && has_interp == is_interp &&
> (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
> arch->flags |= ARM64_ELF_BTI;
> }
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 439ed81e755a..81e151a57df2 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -716,8 +716,9 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> */
>
> static int parse_elf_property(const char *data, size_t *off, size_t datasz,
> - struct arch_elf_state *arch,
> - bool have_prev_type, u32 *prev_type)
> + struct arch_elf_state *arch, bool has_interp,
> + bool is_interp, bool have_prev_type,
> + u32 *prev_type)
> {
> size_t o, step;
> const struct gnu_property *pr;
> @@ -751,7 +752,8 @@ static int parse_elf_property(const char *data, size_t *off, size_t datasz,
> *prev_type = pr->pr_type;
>
> ret = arch_parse_elf_property(pr->pr_type, data + o,
> - pr->pr_datasz, ELF_COMPAT, arch);
> + pr->pr_datasz, ELF_COMPAT,
> + has_interp, is_interp, arch);
> if (ret)
> return ret;
>
> @@ -764,6 +766,7 @@ static int parse_elf_property(const char *data, size_t *off, size_t datasz,
> #define NOTE_NAME_SZ (sizeof(GNU_PROPERTY_TYPE_0_NAME))
>
> static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
> + bool has_interp, bool is_interp,
> struct arch_elf_state *arch)
> {
> union {
> @@ -813,7 +816,8 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
> have_prev_type = false;
> do {
> ret = parse_elf_property(note.data, &off, datasz, arch,
> - have_prev_type, &prev_type);
> + has_interp, is_interp, have_prev_type,
> + &prev_type);
> have_prev_type = true;
> } while (!ret);
>
> @@ -828,6 +832,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> unsigned long error;
> struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
> struct elf_phdr *elf_property_phdata = NULL;
> + struct elf_phdr *interp_elf_property_phdata = NULL;
> unsigned long elf_bss, elf_brk;
> int bss_prot = 0;
> int retval, i;
> @@ -936,6 +941,10 @@ static int load_elf_binary(struct linux_binprm *bprm)
> executable_stack = EXSTACK_DISABLE_X;
> break;
>
> + case PT_GNU_PROPERTY:
> + elf_property_phdata = elf_ppnt;
> + break;
> +
> case PT_LOPROC ... PT_HIPROC:
> retval = arch_elf_pt_proc(elf_ex, elf_ppnt,
> bprm->file, false,
> @@ -963,12 +972,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
> goto out_free_dentry;
>
> /* Pass PT_LOPROC..PT_HIPROC headers to arch code */
> - elf_property_phdata = NULL;
> elf_ppnt = interp_elf_phdata;
> for (i = 0; i < interp_elf_ex->e_phnum; i++, elf_ppnt++)
> switch (elf_ppnt->p_type) {
> case PT_GNU_PROPERTY:
> - elf_property_phdata = elf_ppnt;
> + interp_elf_property_phdata = elf_ppnt;
> break;
>
> case PT_LOPROC ... PT_HIPROC:
> @@ -979,10 +987,17 @@ static int load_elf_binary(struct linux_binprm *bprm)
> goto out_free_dentry;
> break;
> }
> +
> + retval = parse_elf_properties(interpreter,
> + interp_elf_property_phdata,
> + true, true, &arch_state);
> + if (retval)
> + goto out_free_dentry;
> +
> }
>
> - retval = parse_elf_properties(interpreter ?: bprm->file,
> - elf_property_phdata, &arch_state);
> + retval = parse_elf_properties(bprm->file, elf_property_phdata,
> + interpreter != NULL, false, &arch_state);
> if (retval)
> goto out_free_dentry;
>
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index c9a46c4e183b..1c45ecf29147 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -88,13 +88,15 @@ struct arch_elf_state;
> #ifndef CONFIG_ARCH_USE_GNU_PROPERTY
> static inline int arch_parse_elf_property(u32 type, const void *data,
> size_t datasz, bool compat,
> + bool has_interp, bool is_interp,
> struct arch_elf_state *arch)
> {
> return 0;
> }
> #else
> extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
> - bool compat, struct arch_elf_state *arch);
> + bool compat, bool has_interp, bool is_interp,
> + struct arch_elf_state *arch);
> #endif
>
> #ifdef CONFIG_ARCH_HAVE_ELF_PROT
> --
> 2.20.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-07-19 10:06 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-12 11:52 [PATCH v4 0/4] arm64: Enable BTI for the executable as well as the interpreter Mark Brown via Libc-alpha
2021-07-12 11:52 ` [PATCH v4 1/4] elf: Allow architectures to parse properties on the main executable Mark Brown via Libc-alpha
2021-07-19 10:04 ` Dave Martin via Libc-alpha [this message]
2021-07-12 11:52 ` [PATCH v4 2/4] arm64: Enable BTI for main executable as well as the interpreter Mark Brown via Libc-alpha
2021-07-19 10:05 ` Dave Martin via Libc-alpha
2021-07-12 11:52 ` [PATCH v4 3/4] elf: Remove has_interp property from arch_adjust_elf_prot() Mark Brown via Libc-alpha
2021-07-19 10:05 ` Dave Martin via Libc-alpha
2021-07-12 11:52 ` [PATCH v4 4/4] elf: Remove has_interp property from arch_parse_elf_property() Mark Brown via Libc-alpha
2021-07-19 10:05 ` Dave Martin via Libc-alpha
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210719100427.GT4187@arm.com \
--to=libc-alpha@sourceware.org \
--cc=Dave.Martin@arm.com \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=jeremy.linton@arm.com \
--cc=linux-arch@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=szabolcs.nagy@arm.com \
--cc=will@kernel.org \
--cc=yu-cheng.yu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).