* [PATCH v2] x86: Preserve iopl on fork and execve @ 2015-05-11 23:38 Alex Henrie 2015-05-12 6:40 ` Ingo Molnar 0 siblings, 1 reply; 11+ messages in thread From: Alex Henrie @ 2015-05-11 23:38 UTC (permalink / raw) To: One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, linux-kernel Cc: Alex Henrie 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(); } -- 2.4.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-11 23:38 [PATCH v2] x86: Preserve iopl on fork and execve Alex Henrie @ 2015-05-12 6:40 ` Ingo Molnar 2015-05-12 15:13 ` Linus Torvalds 2015-05-12 15:24 ` Arjan van de Ven 0 siblings, 2 replies; 11+ messages in thread From: Ingo Molnar @ 2015-05-12 6:40 UTC (permalink / raw) To: Alex Henrie Cc: One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, linux-kernel, Andy Lutomirski, Linus Torvalds, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-12 6:40 ` Ingo Molnar @ 2015-05-12 15:13 ` Linus Torvalds 2015-05-12 18:24 ` H. Peter Anvin 2015-05-12 15:24 ` Arjan van de Ven 1 sibling, 1 reply; 11+ messages in thread From: Linus Torvalds @ 2015-05-12 15:13 UTC (permalink / raw) To: Ingo Molnar Cc: Alex Henrie, One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, Linux Kernel Mailing List, Andy Lutomirski, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst On Mon, May 11, 2015 at 11:40 PM, Ingo Molnar <mingo@kernel.org> wrote: > > - 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. Indeed. The Linux "no regressions" rule is not about some theoretical "the ABI changed". It's about actual observed regressions. So if we can improve the ABI without any user program or workflow breaking, that's fine. How was this detected? Was it just from code inspection? Because if so, I think the "don't preserve iopl" is indeed the better ABI and we should keep it, accidental or not, since it restricts the impact. But if it turns out somebody was actually depending on it, it's a regression. Of course, 2.5 years later, that is unlikely, but hey, some usages clearly end up updating kernels much too seldom. Linus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-12 15:13 ` Linus Torvalds @ 2015-05-12 18:24 ` H. Peter Anvin 0 siblings, 0 replies; 11+ messages in thread From: H. Peter Anvin @ 2015-05-12 18:24 UTC (permalink / raw) To: Linus Torvalds, Ingo Molnar Cc: Alex Henrie, One Thousand Gnomes, Kees Cook, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, Linux Kernel Mailing List, Andy Lutomirski, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst On 05/12/2015 08:13 AM, Linus Torvalds wrote: > On Mon, May 11, 2015 at 11:40 PM, Ingo Molnar <mingo@kernel.org> wrote: >> >> - 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. > > Indeed. The Linux "no regressions" rule is not about some theoretical > "the ABI changed". It's about actual observed regressions. > > So if we can improve the ABI without any user program or workflow > breaking, that's fine. > But Linus, that would break dominix... ;) -hpa ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-12 6:40 ` Ingo Molnar 2015-05-12 15:13 ` Linus Torvalds @ 2015-05-12 15:24 ` Arjan van de Ven 2015-05-12 15:25 ` Arjan van de Ven 1 sibling, 1 reply; 11+ messages in thread From: Arjan van de Ven @ 2015-05-12 15:24 UTC (permalink / raw) To: Ingo Molnar Cc: Alex Henrie, One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, LKML, Andy Lutomirski, Linus Torvalds, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst On Mon, May 11, 2015 at 11:40 PM, Ingo Molnar <mingo@kernel.org> wrote: > - 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. > - 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. since nothing really broke and its a "nasty either way" regression wise, picking the more secure path looks the most sane. the most likely impact path is in the X world, where X normally gets iopl type permissions (even thought it doesn't need them anymore nowadays).. reverting this behavior would give all the processes X spawns off those perms as well... also the interesting question is: can a process give up these perms? otherwise it becomes a "once given, never gotten rid of" hell hole. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 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-14 10:41 ` Josh Triplett 0 siblings, 2 replies; 11+ messages in thread From: Arjan van de Ven @ 2015-05-12 15:25 UTC (permalink / raw) To: Ingo Molnar Cc: Alex Henrie, One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, LKML, Andy Lutomirski, Linus Torvalds, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst > > also the interesting question is: > can a process give up these perms? > otherwise it becomes a "once given, never gotten rid of" hell hole. If you look at a modern linux distro, nothing should need/use iopl and co anymore, so maybe an interesting question is if we can stick these behind a CONFIG_ option (default on of course for compatibility)... just like some of the /dev/mem like things are now hidable for folks who know they don't need them. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 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-14 10:41 ` Josh Triplett 1 sibling, 1 reply; 11+ messages in thread From: Austin S Hemmelgarn @ 2015-05-12 15:47 UTC (permalink / raw) To: Arjan van de Ven, Ingo Molnar Cc: Alex Henrie, One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, LKML, Andy Lutomirski, Linus Torvalds, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst [-- Attachment #1: Type: text/plain, Size: 815 bytes --] On 2015-05-12 11:25, Arjan van de Ven wrote: >> >> also the interesting question is: >> can a process give up these perms? >> otherwise it becomes a "once given, never gotten rid of" hell hole. > > If you look at a modern linux distro, nothing should need/use iopl and > co anymore, so maybe an interesting > question is if we can stick these behind a CONFIG_ option (default on > of course for compatibility)... just like > some of the /dev/mem like things are now hidable for folks who know > they don't need them. Personally, I _really_ like this idea. The only thing I know of on any modern distro that even considers using ioperm is hwclock, and it only does so if it can't access the RTC through other means (and if you have an RTC, you really should have the /dev interface enabled). [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 2967 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-12 15:47 ` Austin S Hemmelgarn @ 2015-05-12 18:05 ` Alex Henrie 2015-05-12 18:12 ` Austin S Hemmelgarn 0 siblings, 1 reply; 11+ messages in thread From: Alex Henrie @ 2015-05-12 18:05 UTC (permalink / raw) To: Austin S Hemmelgarn Cc: Arjan van de Ven, Ingo Molnar, One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, LKML, Andy Lutomirski, Linus Torvalds, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst 2015-05-12 9:47 GMT-06:00 Austin S Hemmelgarn <ahferroin7@gmail.com>: > On 2015-05-12 11:25, Arjan van de Ven wrote: >> If you look at a modern linux distro, nothing should need/use iopl and >> co anymore, so maybe an interesting >> question is if we can stick these behind a CONFIG_ option (default on >> of course for compatibility)... just like >> some of the /dev/mem like things are now hidable for folks who know >> they don't need them. > > Personally, I _really_ like this idea. The only thing I know of on any > modern distro that even considers using ioperm is hwclock, and it only does > so if it can't access the RTC through other means (and if you have an RTC, > you really should have the /dev interface enabled). Removing iopl might be OK. Removing ioperm would break my use case of legacy code that needs direct access to the parallel port. -Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-12 18:05 ` Alex Henrie @ 2015-05-12 18:12 ` Austin S Hemmelgarn 0 siblings, 0 replies; 11+ messages in thread From: Austin S Hemmelgarn @ 2015-05-12 18:12 UTC (permalink / raw) To: Alex Henrie Cc: Arjan van de Ven, Ingo Molnar, One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, Tyler Hicks, Al Viro, LKML, Andy Lutomirski, Linus Torvalds, Andrew Morton, Borislav Petkov, Peter Zijlstra, Arjan van de Ven, Denys Vlasenko, Brian Gerst [-- Attachment #1: Type: text/plain, Size: 1279 bytes --] On 2015-05-12 14:05, Alex Henrie wrote: > 2015-05-12 9:47 GMT-06:00 Austin S Hemmelgarn <ahferroin7@gmail.com>: >> On 2015-05-12 11:25, Arjan van de Ven wrote: >>> If you look at a modern linux distro, nothing should need/use iopl and >>> co anymore, so maybe an interesting >>> question is if we can stick these behind a CONFIG_ option (default on >>> of course for compatibility)... just like >>> some of the /dev/mem like things are now hidable for folks who know >>> they don't need them. >> >> Personally, I _really_ like this idea. The only thing I know of on any >> modern distro that even considers using ioperm is hwclock, and it only does >> so if it can't access the RTC through other means (and if you have an RTC, >> you really should have the /dev interface enabled). > > Removing iopl might be OK. Removing ioperm would break my use case of > legacy code that needs direct access to the parallel port. > > -Alex > The discussion isn't about outright removing them, just providing a config option to disable them. It might be a good idea though to provide separate config options for each of iopl() and ioperm(), as iopl() is more dangerous, and ioperm() is more widely used, and people may need one but not want to have the other. [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 2967 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-12 15:25 ` Arjan van de Ven 2015-05-12 15:47 ` Austin S Hemmelgarn @ 2015-05-14 10:41 ` Josh Triplett 2015-05-15 0:52 ` H. Peter Anvin 1 sibling, 1 reply; 11+ messages in thread From: Josh Triplett @ 2015-05-14 10:41 UTC (permalink / raw) To: Arjan van de Ven Cc: Ingo Molnar, Alex Henrie, One Thousand Gnomes, Kees Cook, H . Peter Anvin, Doug Johnson, Thomas Gleixner, Ingo Molnar, linux-kernel On Tue, May 12, 2015 at 08:25:59AM -0700, Arjan van de Ven wrote: > > also the interesting question is: > > can a process give up these perms? > > otherwise it becomes a "once given, never gotten rid of" hell hole. > > If you look at a modern linux distro, nothing should need/use iopl and > co anymore, so maybe an interesting > question is if we can stick these behind a CONFIG_ option (default on > of course for compatibility)... just like > some of the /dev/mem like things are now hidable for folks who know > they don't need them. I have a patch series that does exactly that, compiling out the syscalls as well as the underlying architecture-specific infrastructure. (Saves quite a bit of space, too.) It still needs some more detailed x86 architecture review. Peter, Ingo? Would you be interested in taking (an updated version of) that patch series for the next merge window? - Josh Triplett ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86: Preserve iopl on fork and execve 2015-05-14 10:41 ` Josh Triplett @ 2015-05-15 0:52 ` H. Peter Anvin 0 siblings, 0 replies; 11+ messages in thread From: H. Peter Anvin @ 2015-05-15 0:52 UTC (permalink / raw) To: Josh Triplett, Arjan van de Ven Cc: Ingo Molnar, Alex Henrie, One Thousand Gnomes, Kees Cook, Doug Johnson, Thomas Gleixner, Ingo Molnar, linux-kernel On 05/14/2015 03:41 AM, Josh Triplett wrote: > > I have a patch series that does exactly that, compiling out the syscalls > as well as the underlying architecture-specific infrastructure. (Saves > quite a bit of space, too.) > > It still needs some more detailed x86 architecture review. Peter, Ingo? > Would you be interested in taking (an updated version of) that patch > series for the next merge window? > I think that makes sense. -hpa ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-05-15 0:53 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-11 23:38 [PATCH v2] x86: Preserve iopl on fork and execve Alex Henrie 2015-05-12 6:40 ` Ingo Molnar 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
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).