From: Ingo Molnar <mingo@kernel.org>
To: Alex Henrie <alexhenrie24@gmail.com>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>,
Kees Cook <keescook@chromium.org>,
"H . Peter Anvin" <hpa@zytor.com>,
Doug Johnson <dougvj@gmail.com>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>,
Tyler Hicks <tyhicks@canonical.com>,
Al Viro <viro@zeniv.linux.org.uk>,
linux-kernel@vger.kernel.org, Andy Lutomirski <luto@kernel.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrew Morton <akpm@linux-foundation.org>,
Borislav Petkov <bp@alien8.de>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Arjan van de Ven <arjan@infradead.org>,
Denys Vlasenko <dvlasenk@redhat.com>,
Brian Gerst <brgerst@gmail.com>
Subject: Re: [PATCH v2] x86: Preserve iopl on fork and execve
Date: Tue, 12 May 2015 08:40:32 +0200 [thread overview]
Message-ID: <20150512064032.GA25097@gmail.com> (raw)
In-Reply-To: <1431387505-13410-1-git-send-email-alexhenrie24@gmail.com>
* Alex Henrie <alexhenrie24@gmail.com> wrote:
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> Suggested-by: Doug Johnson <dougvj@dougvj.net>
> ---
> arch/x86/kernel/process_32.c | 2 +-
> arch/x86/kernel/process_64.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 8ed2106..0ef7078 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -205,7 +205,7 @@ start_thread(struct pt_regs *regs, unsigned long new_ip, unsigned long new_sp)
> regs->cs = __USER_CS;
> regs->ip = new_ip;
> regs->sp = new_sp;
> - regs->flags = X86_EFLAGS_IF;
> + regs->flags = X86_EFLAGS_IF | (X86_EFLAGS_IOPL & regs->flags);
> force_iret();
> }
> EXPORT_SYMBOL_GPL(start_thread);
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index ddfdbf7..e21eda2 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -238,7 +238,7 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip,
> regs->sp = new_sp;
> regs->cs = _cs;
> regs->ss = _ss;
> - regs->flags = X86_EFLAGS_IF;
> + regs->flags = X86_EFLAGS_IF | (X86_EFLAGS_IOPL & regs->flags);
> force_iret();
> }
Yeah, NAK.
So this patch could be an instant roothole on some setups: assume old
64-bit apps relying on fork/clone/execve effectively flushing these
capabilities and we'll now leak powerful hardware access permissions
into child contexts that never had it before ...
I realize that this is a 2.5+ years old regression on 32-bit x86, and
that the prior inheritance of iopl/ioperm was broken accidentally on
32-bit kernels by:
6783eaa2e125 ("x86, um/x86: switch to generic sys_execve and kernel_execve")
My arguments in favor of doing nothing are:
- Nothing actually broke that people cared about in the last 2.5
years, thus this might be one of the (very very rare) cases where
preserving a breakage is the right thing to do.
- There's no reason to export this behavior to 64-bit x86 which
apparently never had the iopl/ioperm capabilities propagation.
- Furthermore, even new 32-bit apps might have (accidentally) learned
the new ABI, and we'd now break _them_, possibly in subtle ways.
- Plus iopl() and ioperm() are one of the most dangerous kernel APIs
we have and the accidental limiting of them, which we got away with
for 2.5+ years without being reportd, might just be what we want to
stick with. An aspect of an API is only an ABI if it's actually
used by applications.
- These syscalls are rarely used, and we could as well insist that
every new context should have the permissions to (re-)acquire them
and should actively seek them - instead of inheriting it to shells
via system(), etc. The best strategy with dangerous APIs is to make
it really, really explicit when they are used.
Permission propagation breakages like this are a rare situation and
there's really no good way to fix them: damned if you do, damned if
you don't.
So without far more analysis and far more care (a zero-length
changelog won't cut it!) I doubt we can - or event want to - do
anything like this...
Thanks,
Ingo
next prev parent reply other threads:[~2015-05-12 6:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-11 23:38 [PATCH v2] x86: Preserve iopl on fork and execve Alex Henrie
2015-05-12 6:40 ` Ingo Molnar [this message]
2015-05-12 15:13 ` Linus Torvalds
2015-05-12 18:24 ` H. Peter Anvin
2015-05-12 15:24 ` Arjan van de Ven
2015-05-12 15:25 ` Arjan van de Ven
2015-05-12 15:47 ` Austin S Hemmelgarn
2015-05-12 18:05 ` Alex Henrie
2015-05-12 18:12 ` Austin S Hemmelgarn
2015-05-14 10:41 ` Josh Triplett
2015-05-15 0:52 ` H. Peter Anvin
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=20150512064032.GA25097@gmail.com \
--to=mingo@kernel.org \
--cc=a.p.zijlstra@chello.nl \
--cc=akpm@linux-foundation.org \
--cc=alexhenrie24@gmail.com \
--cc=arjan@infradead.org \
--cc=bp@alien8.de \
--cc=brgerst@gmail.com \
--cc=dougvj@gmail.com \
--cc=dvlasenk@redhat.com \
--cc=gnomes@lxorguk.ukuu.org.uk \
--cc=hpa@zytor.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=tyhicks@canonical.com \
--cc=viro@zeniv.linux.org.uk \
/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 NNTP newsgroup(s).