Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1632979 > unrolled thread
| Started by | Thomas Garnier <thgarnie@google.com> |
|---|---|
| First post | 2017-04-28 17:40 +0200 |
| Last post | 2017-05-09 08:40 +0200 |
| Articles | 7 on this page of 47 — 13 participants |
Back to article view | Back to linux.kernel
[PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-04-28 17:40 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-06 00:20 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 09:40 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 10:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Daniel Micay <danielmicay@gmail.com> - 2017-05-08 17:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 17:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-08 22:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-09 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Greg KH <greg@kroah.com> - 2017-05-09 13:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Thomas Garnier <thgarnie@google.com> - 2017-05-09 16:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-09 18:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Greg KH <greg@kroah.com> - 2017-05-08 14:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-09 08:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-09 11:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-09 15:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-09 15:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-09 18:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-09 19:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@amacapital.net> - 2017-05-10 01:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-10 01:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-10 04:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Borislav Petkov <bp@alien8.de> - 2017-05-11 13:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 08:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 04:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 04:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 05:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 05:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 05:50 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 09:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:00 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-10 09:30 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 09:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Brian Gerst <brgerst@gmail.com> - 2017-05-09 18:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Arnd Bergmann <arnd@arndb.de> - 2017-05-10 09:40 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-10 10:10 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Christoph Hellwig <hch@infradead.org> - 2017-05-10 10:20 +0200
Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Andy Lutomirski <luto@kernel.org> - 2017-05-11 02:20 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 15:20 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Jann Horn <jannh@google.com> - 2017-05-08 16:10 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-08 22:50 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-08 16:10 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Kees Cook <keescook@chromium.org> - 2017-05-08 17:30 +0200
Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode Ingo Molnar <mingo@kernel.org> - 2017-05-09 08:40 +0200
Page 3 of 3 — ← Prev page 1 2 [3]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-05-11 02:20 +0200 |
| Subject | Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tFJZM-7uS-3@gated-at.bofh.it> |
| In reply to | #1638647 |
On Wed, May 10, 2017 at 1:14 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Wed, May 10, 2017 at 09:08:41AM +0100, Al Viro wrote: >> On Wed, May 10, 2017 at 09:37:04AM +0200, Arnd Bergmann wrote: >> >> > > How about trying to remove all of them? If we could actually get rid >> > > of all of them, we could drop the arch support, and we'd get faster, >> > > simpler, shorter uaccess code throughout the kernel. >> >> BTW, not all get_user() under KERNEL_DS are plain loads. There is an >> exception - probe_kernel_read(). > > And various calls that looks like opencoded versions, e.g. drivers/dio > or the ELF loader. > > But in the long run we'll just need a separate primitive for that, > but that can wait until the set_fs calls outside the core code are > gone. I suspect that, on most arches, the primitive is called __copy_from_user(). We could make the generic code do that except where overridden.
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-05-08 15:20 +0200 |
| Message-ID | <tEQJY-4ot-27@gated-at.bofh.it> |
| In reply to | #1637273 |
On Mon, May 8, 2017 at 12:33 AM, Ingo Molnar <mingo@kernel.org> wrote: > > (added more Cc:s) > > * Thomas Garnier <thgarnie@google.com> wrote: > >> On Fri, Apr 28, 2017 at 8:32 AM, Thomas Garnier <thgarnie@google.com> wrote: >> > Ensure that a syscall does not return to user-mode with a kernel address >> > limit. If that happens, a process can corrupt kernel-mode memory and >> > elevate privileges [1]. >> > >> > The CONFIG_ADDR_LIMIT_CHECK option disables the generic check so each >> > architecture can create optimized versions. This option is enabled by >> > default on s390 because a similar feature already exists. >> > >> > [1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990 >> > >> > Signed-off-by: Thomas Garnier <thgarnie@google.com> >> > Tested-by: Kees Cook <keescook@chromium.org> >> >> Ingo: Do you want to take the set? > > Yeah, so now I'm questioning the whole premise of the feature, sorry :-/ > > A big disavantage is that the "security check" will add 2-5 instructions to the > system call fast path. Every one of them, and essentially forever. Just to handle > a CVE that was caused by a buggy touch-screen driver helper function leaking > KERNEL_DS and which was fixed long ago ... > > And yes, I realize that there were other such bugs and that such bugs might occur > in the future - but why not push the overhead of the security check to the kernel > build phase? I.e. I'm wondering how well we could do static analysis during kernel > build - would a limited mode of Sparse be good enough for that? Or we could add a > new static checker to tools/, built from first principles and used primarily for > extended syntactical checking. Static analysis is just not going to cover all cases. We've had vulnerabilities where interrupt handlers left KERNEL_DS set, for example. If there are performance concerns, let's put this behind a CONFIG. 2-5 instructions is not an issue for most people that want this coverage. > For example I'd consider it a good practice to mandate that if a kernel function > sets KERNEL_DS then it must restore it as well. Any function that does not do > that, or is too complex for the static analysis to prove correctness for sure > should be considered buggy! > > Are there any common kernel APIs outside set_fs() that set KERNEL_DS > intentionally? The overwhelming pattern ought to be: > > orig_fs = get_fs(); > set_fs(KERNEL_DS); > ... > set_fs(orig_fs); > > ... and even a relatively simple static analysis tool ought to be able to see > through that. This pattern was, in fact, what the interrupt handler bug escaped from. We have to build proactive defenses, and this check has a clear defensive advantage. It's a noble goal to improve the static analyzers and simplify the source, but we have too much history to prove that this just isn't enough. This instruction cost of this is extremely small, too. Until we can eliminate set_fs(), we need to add this check. > I'd even suggest we do it not like Sparse builds are done today, but in a more > integrated fashion: do static analysis as part of a typical kernel defconfig build > and not tolerate warnings but go for a 'zero warnings' policy like Linus uses for > modconfig builds. > > _That_ solution I'd feel very, very good about - it would be so much better than > any runtime checks... I'm not opposed to this, but there will be push-back on "making the build slower", and it still won't catch everything. Bug-finding is different from making a bug class just unexploitable at all. As we've done before, it's the difference between trying to find format string attacks vs just removing %n from the format parser. > Not to mention that such an integrated static analysis facility would allow many > other things to be checked during build time, which we couldn't possibly check > runtime. Absolutely! But it's orthogonal to proactive runtime exploit blocking. We've got one that works and defends against an entire class of vulnerability for very low cost. It it's truly too costly for default, let's put it behind a CONFIG and see who wants it. (Most distros, I suspect, will enable it, just like hardened usercopy which is much more expensive than this.) -Kees -- Kees Cook Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Jann Horn <jannh@google.com> |
|---|---|
| Date | 2017-05-08 16:10 +0200 |
| Message-ID | <tERwm-4V1-9@gated-at.bofh.it> |
| In reply to | #1637440 |
On Mon, May 8, 2017 at 4:02 PM, Ingo Molnar <mingo@kernel.org> wrote: > > * Kees Cook <keescook@chromium.org> wrote: > >> > And yes, I realize that there were other such bugs and that such bugs might >> > occur in the future - but why not push the overhead of the security check to >> > the kernel build phase? I.e. I'm wondering how well we could do static >> > analysis during kernel build - would a limited mode of Sparse be good enough >> > for that? Or we could add a new static checker to tools/, built from first >> > principles and used primarily for extended syntactical checking. >> >> Static analysis is just not going to cover all cases. We've had vulnerabilities >> where interrupt handlers left KERNEL_DS set, for example. [...] > > Got any commit ID of that bug - was it because a function executed by the > interrupt handler leaked KERNEL_DS? I think Kees might be talking about https://bugs.chromium.org/p/project-zero/issues/detail?id=822, fixed in commit e6978e4bf181fb3b5f8cb6f71b4fe30fbf1b655c. The issue was that perf code that can run in pretty much any context called access_ok().
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-05-08 22:50 +0200 |
| Subject | Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tEXLr-ls-1@gated-at.bofh.it> |
| In reply to | #1637458 |
On Mon, May 08, 2017 at 04:06:35PM +0200, Jann Horn wrote: > I think Kees might be talking about > https://bugs.chromium.org/p/project-zero/issues/detail?id=822, fixed in > commit e6978e4bf181fb3b5f8cb6f71b4fe30fbf1b655c. The issue was that > perf code that can run in pretty much any context called access_ok(). And that commit has *NOT* solved the problem. perf_callchain_user() can be called synchronously, without passing through that code. Tracepoint shite... That set_fs() should be done in get_perf_callchain(), just around the call of perf_callchain_user(). Along with pagefault_disable(), actually. BTW, that's a nice example demonstrating why doing that on the kernel boundary is wrong. Wider (in theory) area being "protected" => easier to miss the ways not crossing its border.
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-05-08 16:10 +0200 |
| Subject | Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tERwm-4V1-7@gated-at.bofh.it> |
| In reply to | #1637440 |
* Kees Cook <keescook@chromium.org> wrote: > > And yes, I realize that there were other such bugs and that such bugs might > > occur in the future - but why not push the overhead of the security check to > > the kernel build phase? I.e. I'm wondering how well we could do static > > analysis during kernel build - would a limited mode of Sparse be good enough > > for that? Or we could add a new static checker to tools/, built from first > > principles and used primarily for extended syntactical checking. > > Static analysis is just not going to cover all cases. We've had vulnerabilities > where interrupt handlers left KERNEL_DS set, for example. [...] Got any commit ID of that bug - was it because a function executed by the interrupt handler leaked KERNEL_DS? > [...] If there are performance concerns, let's put this behind a CONFIG. 2-5 > instructions is not an issue for most people that want this coverage. That doesn't really _solve_ the performance concerns, it just forces most people to enable it by creating a 'security or performance' false dichotomy ... > [...] and it still won't catch everything. Bug-finding is different from making > a bug class just unexploitable at all. As we've done before, it's the difference > between trying to find format string attacks vs just removing %n from the format > parser. No, it does not make it unexploitable, it could still be exploitable if the runtime check is buggy or if there's kernel execution outside of the regular system call paths - there's plenty of such hardware functionality on x86 for example. Thanks, Ingo
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-05-08 17:30 +0200 |
| Message-ID | <tESLM-5F6-25@gated-at.bofh.it> |
| In reply to | #1637459 |
On Mon, May 8, 2017 at 7:02 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Kees Cook <keescook@chromium.org> wrote: > >> > And yes, I realize that there were other such bugs and that such bugs might >> > occur in the future - but why not push the overhead of the security check to >> > the kernel build phase? I.e. I'm wondering how well we could do static >> > analysis during kernel build - would a limited mode of Sparse be good enough >> > for that? Or we could add a new static checker to tools/, built from first >> > principles and used primarily for extended syntactical checking. >> >> Static analysis is just not going to cover all cases. We've had vulnerabilities >> where interrupt handlers left KERNEL_DS set, for example. [...] > > Got any commit ID of that bug - was it because a function executed by the > interrupt handler leaked KERNEL_DS? Ah, it was an exception handler, but the one I was thinking of was this: https://lwn.net/Articles/419141/ >> [...] If there are performance concerns, let's put this behind a CONFIG. 2-5 >> instructions is not an issue for most people that want this coverage. > > That doesn't really _solve_ the performance concerns, it just forces most people > to enable it by creating a 'security or performance' false dichotomy ... That's fair, but what I'm trying to say is that many people will want this, so rejecting it because it's 2 more instructions seems unreasonable. We have had much more invasive changes added to the kernel. >> [...] and it still won't catch everything. Bug-finding is different from making >> a bug class just unexploitable at all. As we've done before, it's the difference >> between trying to find format string attacks vs just removing %n from the format >> parser. > > No, it does not make it unexploitable, it could still be exploitable if the > runtime check is buggy or if there's kernel execution outside of the regular > system call paths - there's plenty of such hardware functionality on x86 for > example. Fine, but this is splitting hairs. This does protect a specific situation, and it does so very cheaply. The real fix would be to remove set_fs() entirely. :P -Kees -- Kees Cook Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-05-09 08:40 +0200 |
| Subject | Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode |
| Message-ID | <tF6Yp-6mH-1@gated-at.bofh.it> |
| In reply to | #1637511 |
* Kees Cook <keescook@chromium.org> wrote: > On Mon, May 8, 2017 at 7:02 AM, Ingo Molnar <mingo@kernel.org> wrote: > > > > * Kees Cook <keescook@chromium.org> wrote: > > > >> > And yes, I realize that there were other such bugs and that such bugs might > >> > occur in the future - but why not push the overhead of the security check to > >> > the kernel build phase? I.e. I'm wondering how well we could do static > >> > analysis during kernel build - would a limited mode of Sparse be good enough > >> > for that? Or we could add a new static checker to tools/, built from first > >> > principles and used primarily for extended syntactical checking. > >> > >> Static analysis is just not going to cover all cases. We've had vulnerabilities > >> where interrupt handlers left KERNEL_DS set, for example. [...] > > > > Got any commit ID of that bug - was it because a function executed by the > > interrupt handler leaked KERNEL_DS? > > Ah, it was an exception handler, but the one I was thinking of was this: > https://lwn.net/Articles/419141/ Ok, so that's CVE-2010-4258, where an oops with KERNEL_DS set was used to escalate privileges, due to the kernel's oops handler not cleaning up the KERNEL_DS. The exploit used another bug, a crash in a network protocol handler, to execute the oops handler with KERNEL_DS set. The explanation of the exploit itself points out that it's a very interesting bug and I agree, it's not a general kernel bug but a bug in a very narrow code path (oops handling) that caused this, and I don't see how that example can be turned into a general example: it was a bug in oops handling to let the process continue execution (and perform the CLEARTID operation) *and* leak the address limit at KERNEL_DS. By similar argument a bug in the runtime checking of the address limit may allow exploits. Consider the oops path cleanup a similarly sensitive code path as the address limit check. To handle this category of exploits it would be enough to add a runtime check to the _oops handling code itself_ (to make sure we've set addr_limit back to USER_DS even if we crash in a KERNEL_DS code area), not to every system call! That check would avoid that particular historic pattern, if combined with static analysis that ensured that KERNEL_DS is always set/restored correctly. (Which btw. I believe some of the regular static scans of the kernel are already doing today.) Furthermore, to go back to your original argument: > Static analysis is just not going to cover all cases. it's not even true that a runtime check will 'cover all cases': for example a similar bug to CVE-2010-4258 could still be exploited: - Note that the actual put_user() was not prevented via the runtime check - the runtime check would run *after* the buggy put_user() was done. The runtime check warns or panics after the fact, which might (or might not) be enough to prevent the exploit. - Also note that a slightly different form of the bug would still be exploitable, even with the runtime check: for example if the task-shutdown code can be made to unconditionally set KERNEL_DS, but after the put_user(), then the runtime check would not 'cover all cases'. So the argument for doing this runtime check after every system call is very dubious. Thanks, Ingo
[toc] | [prev] | [standalone]
Page 3 of 3 — ← Prev page 1 2 [3]
Back to top | Article view | linux.kernel
csiph-web