From: Will Deacon <will@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: Mark Brown <broonie@kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Szabolcs Nagy <szabolcs.nagy@arm.com>,
Jeremy Linton <jeremy.linton@arm.com>,
"H . J . Lu" <hjl.tools@gmail.com>,
Yu-cheng Yu <yu-cheng.yu@intel.com>,
Eric Biederman <ebiederm@xmission.com>,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
libc-alpha@sourceware.org, Dave Martin <Dave.Martin@arm.com>
Subject: Re: [PATCH v10 1/2] elf: Allow architectures to parse properties on the main executable
Date: Tue, 8 Mar 2022 09:45:22 +0000 [thread overview]
Message-ID: <20220308094521.GA31063@willie-the-truck> (raw)
In-Reply-To: <202203071551.DBABE01@keescook>
On Mon, Mar 07, 2022 at 04:00:15PM -0800, Kees Cook wrote:
> On Mon, Feb 28, 2022 at 01:06:05PM +0000, 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 a new
> > flag is_interp flag as with arch_elf_adjust_prot() and calling it for
> > both the main executable and any intepreter.
> >
> > The user of this code, arm64, is adapted to ensure that there is no
> > functional change.
> >
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > Tested-by: Jeremy Linton <jeremy.linton@arm.com>
> > Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> > arch/arm64/include/asm/elf.h | 3 ++-
> > fs/binfmt_elf.c | 32 +++++++++++++++++++++++---------
> > include/linux/elf.h | 4 +++-
> > 3 files changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
> > index 97932fbf973d..5cc002376abe 100644
> > --- a/arch/arm64/include/asm/elf.h
> > +++ b/arch/arm64/include/asm/elf.h
> > @@ -259,6 +259,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)
>
> Adding more and more args to a functions like this gives me the sense
> that some kind of argument structure is needed.
>
> Once I get enough unit testing written in here, I'm hoping to refactor
> a bunch of this. To the future! :)
>
> > @@ -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;
> > @@ -865,6 +870,9 @@ static int load_elf_binary(struct linux_binprm *bprm)
> > for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) {
> > char *elf_interpreter;
> >
> > + if (interpreter && elf_property_phdata)
> > + break;
> > +
>
> This is not okay. This introduces a memory resource leak for malicious
> ELF files with multiple INTERP headers.
>
> > if (elf_ppnt->p_type == PT_GNU_PROPERTY) {
> > elf_property_phdata = elf_ppnt;
> > continue;
> > @@ -919,7 +927,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> > if (retval < 0)
> > goto out_free_dentry;
> >
> > - break;
> > + continue;
>
> Because of this.
>
> As a fix, I'd expect the PT_INTERP test to be updated:
>
> if (interpreter || elf_ppnt->p_type != PT_INTERP)
> continue;
Thanks, Kees. I'll drop this branch from -next until it's been resolved.
Will
next prev parent reply other threads:[~2022-03-08 9:45 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-28 13:06 [PATCH v10 0/2] arm64: Enable BTI for the executable as well as the interpreter Mark Brown
2022-02-28 13:06 ` [PATCH v10 1/2] elf: Allow architectures to parse properties on the main executable Mark Brown
2022-03-08 0:00 ` Kees Cook
2022-03-08 9:45 ` Will Deacon [this message]
2022-03-08 12:21 ` Mark Brown
2022-02-28 13:06 ` [PATCH v10 2/2] arm64: Enable BTI for main executable as well as the interpreter Mark Brown
2022-02-28 18:24 ` Catalin Marinas
2022-03-07 22:03 ` [PATCH v10 0/2] arm64: Enable BTI for the " Will Deacon
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220308094521.GA31063@willie-the-truck \
--to=will@kernel.org \
--cc=Dave.Martin@arm.com \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=ebiederm@xmission.com \
--cc=hjl.tools@gmail.com \
--cc=jeremy.linton@arm.com \
--cc=keescook@chromium.org \
--cc=libc-alpha@sourceware.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=szabolcs.nagy@arm.com \
--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).