* Re: BUGreport: fix minor infoleak in get_user_ex() [not found] <20161027193210.GA23006@zipoli.ccur.com> @ 2016-10-28 0:03 ` Al Viro 2016-10-28 2:02 ` [4.1 backport trouble] " Al Viro 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2016-10-28 0:03 UTC (permalink / raw) To: Joe Korty; +Cc: linux-kernel On Thu, Oct 27, 2016 at 03:32:10PM -0400, Joe Korty wrote: > Hi Al, > I don't know if this is worth fixing or not, but I thought > I would mention it in case it was. > > A git bisect search shows that the commit: > > commit 319fe11519401e8a5db191a0a93aa2c1d7bb59f4 > Author: Al Viro <viro@ZenIV.linux.org.uk> > Date: Thu Sep 15 02:35:29 2016 +0100 > > causes some malformed rt_sigqueueinfo syscalls, executed under > x86_64 kernels running compat mode programs, to oops with > the following message: > > [ 66.054786] BUG: unable to handle kernel paging request at 00000000020573eb > [ 66.061793] IP: [<00000000020573eb>] 0x20573eb > [ 66.066251] PGD 122263067 PUD 120a0c067 PMD 0 > [ 66.070745] Oops: 0010 [#1] PREEMPT SMP > [ 66.074717] Modules linked in: > [ 66.077789] CPU: 7 PID: 5496 Comm: cc Not tainted 4.1.35 #1 > [ 66.083365] Hardware name: Supermicro H8DM8-2/H8DM8-2, BIOS 080014 10/22/2009 > [ 66.090582] task: ffff88006b044400 ti: ffff88006b300000 task.ti: ffff88006b300000 > [ 66.098067] RIP: 0010:[<00000000020573eb>] [<00000000020573eb>] 0x20573eb > [ 66.104961] RSP: 0018:ffff88006b303e98 EFLAGS: 00010246 > [ 66.110269] RAX: 00007fffffffef80 RBX: 0000000000000000 RCX: 0000000000000000 > [ 66.117399] RDX: ffff88006b304000 RSI: 0000000000000000 RDI: ffff88006b303ea8 > [ 66.124528] RBP: ffff88006b303e98 R08: 0000000000000000 R09: 0000000000000000 > [ 66.131667] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 > [ 66.138805] R13: ffff88006b303ea8 R14: 0000000000000000 R15: 0000000000000000 > [ 66.145935] FS: 00007ffff7fca740(0000) GS:ffff880127d80000(0063) knlGS:00000000f7df06c0 > [ 66.154027] CS: 0010 DS: 002b ES: 002b CR0: 000000008005003b > [ 66.159777] CR2: 00000000020573eb CR3: 000000011f874000 CR4: 00000000000006e0 > [ 66.166906] Stack: > [ 66.168919] ffff88006b303f48 ffffffff8107c4b5 0000000000000000 0000000000000000 > [ 66.176403] 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > [ 66.183872] 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > [ 66.191341] Call Trace: > [ 66.193802] [<ffffffff8107c4b5>] compat_SyS_rt_sigqueueinfo+0x45/0x70 > [ 66.200340] [<ffffffff8205648c>] cstar_dispatch+0x7/0x2a > [ 66.205755] Code: Bad RIP value. > [ 66.209103] RIP [<00000000020573eb>] 0x20573eb > [ 66.213648] RSP <ffff88006b303e98> > [ 66.217134] CR2: 00000000020573eb > [ 66.220505] ---[ end trace 4f88266d7fd7e6d7 ]--- > The following test program can be used to trigger the problem: > > /* gcc -m32 c.c -o c */ > #define _GNU_SOURCE > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > #include <errno.h> > #include <sys/syscall.h> > > #define rt_sigqueueinfo 178 > > int main(int argc, char **argv) { > int stat = syscall(rt_sigqueueinfo, 0, 0, 0, 0, 0, 0); > printf("syscall(%d): stat: %d, errno: %d\n", > rt_sigqueueinfo, stat, errno); > return 0; > } > > This is under 4.1.35 on x86_64. AFAICS, it steps on _ASM_EXTABLE_EX being more brittle in 4.1 - it pretty much has to have the handler on the next insn after the faulting one, or the resulting extable entry won't be recognized. This "x86/mm: Expand the exception table logic to allow new handling options" in mainline is where that requirement has disappeared. I think we ought to use the plain _ASM_EXTABLE and just call something that would set current_thread_info()->uaccess_err directly from the fixup code there. That, or backport the commit switching to less brittle extables. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [4.1 backport trouble] Re: BUGreport: fix minor infoleak in get_user_ex() 2016-10-28 0:03 ` BUGreport: fix minor infoleak in get_user_ex() Al Viro @ 2016-10-28 2:02 ` Al Viro [not found] ` <20161028164033.GA29952@zipoli.ccur.com> 2016-10-30 18:16 ` Levin, Alexander 0 siblings, 2 replies; 7+ messages in thread From: Al Viro @ 2016-10-28 2:02 UTC (permalink / raw) To: Joe Korty; +Cc: linux-kernel, Linus Torvalds, Sasha Levin On Fri, Oct 28, 2016 at 01:03:55AM +0100, Al Viro wrote: > On Thu, Oct 27, 2016 at 03:32:10PM -0400, Joe Korty wrote: [oops in 4.1.35, bisected to 319fe1151940] > > The following test program can be used to trigger the problem: > > > > /* gcc -m32 c.c -o c */ > > #define _GNU_SOURCE > > #include <stdio.h> > > #include <stdlib.h> > > #include <unistd.h> > > #include <errno.h> > > #include <sys/syscall.h> > > > > #define rt_sigqueueinfo 178 > > > > int main(int argc, char **argv) { > > int stat = syscall(rt_sigqueueinfo, 0, 0, 0, 0, 0, 0); > > printf("syscall(%d): stat: %d, errno: %d\n", > > rt_sigqueueinfo, stat, errno); > > return 0; > > } > > > > This is under 4.1.35 on x86_64. > > AFAICS, it steps on _ASM_EXTABLE_EX being more brittle in 4.1 - it pretty > much has to have the handler on the next insn after the faulting one, or > the resulting extable entry won't be recognized. This > "x86/mm: Expand the exception table logic to allow new handling options" > in mainline is where that requirement has disappeared. I think we > ought to use the plain _ASM_EXTABLE and just call something that would > set current_thread_info()->uaccess_err directly from the fixup code there. > That, or backport the commit switching to less brittle extables. ... and frankly, backporting 548acf19234d would be my preference. It's a bit more intrusive than needed (_ASM_EXTABLE_FAULT is used only in memcpy_mcsafe(), which is used only by pmem and it's the only reason for passing the trap number to fixup_exception()), but AFAICS it's fairly safe. Objections? ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <20161028164033.GA29952@zipoli.ccur.com>]
* Re: [4.1 backport trouble] Re: BUGreport: fix minor infoleak in get_user_ex() [not found] ` <20161028164033.GA29952@zipoli.ccur.com> @ 2016-10-28 18:21 ` Linus Torvalds 2016-10-28 19:49 ` Al Viro 2016-10-28 19:34 ` Al Viro 1 sibling, 1 reply; 7+ messages in thread From: Linus Torvalds @ 2016-10-28 18:21 UTC (permalink / raw) To: Joe Korty; +Cc: Al Viro, Linux Kernel Mailing List, Sasha Levin, stable rea On Fri, Oct 28, 2016 at 9:40 AM, Joe Korty <joe.korty@concurrent.com> wrote: > > Backporting 548acf19234d to 4.1.35 does indeed fix the > issue. However, it is not clear to my _why_ it works, > so it might be better that someone else push the backport > to stable. The problem is that the old _ASM_EXTABLE_EXT hackery ends up being this code in fixup_exception() back in 4.1 (and later). if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) { /* Special hack for uaccess_err */ current_thread_info()->uaccess_err = 1; new_ip -= 0x7ffffff0; } and it really does depend very intimately on the relationship with the "fixup" address (fixup->fixup) with the instruction that took the fault (fixup->insn). Now, back in the original 4.1 days, that fixup-vs-insn relationship was trivially always the case, since __get_user_asm_ex() always just made the fixup be to fall through to the next instruction. However, when commit 1c109fabbd51 ("fix minor infoleak in get_user_ex()") was backported, now the fixup for __get_user_asm_ex() ends up being in a different section entirely (".section .fixup"), and the close relationship between the faulting instruction and the fixup instruction went away. End result: commit 1c109fabbd51lly effectively and very subtly depends on commit 548acf19234d (introduced in v4.6) that gets rid of the special hack. Adding "stable" to the cc, because this might well affect other stable backports than 4.1. End result: either commit 1c109fabbd51 shouldn't be backported (it's really not that important - if people properly check the exception error results it shouldn't matter), or you need to also backport 548acf19234d as Al suggested. I'd be inclined to say "don't backport 1c109fabbd51", but it's really a judgment call. Linus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [4.1 backport trouble] Re: BUGreport: fix minor infoleak in get_user_ex() 2016-10-28 18:21 ` Linus Torvalds @ 2016-10-28 19:49 ` Al Viro 2016-10-28 21:29 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2016-10-28 19:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: Joe Korty, Linux Kernel Mailing List, Sasha Levin, stable On Fri, Oct 28, 2016 at 11:21:24AM -0700, Linus Torvalds wrote: > End result: either commit 1c109fabbd51 shouldn't be backported (it's > really not that important - if people properly check the exception > error results it shouldn't matter), or you need to also backport > 548acf19234d as Al suggested. > > I'd be inclined to say "don't backport 1c109fabbd51", but it's really > a judgment call. *nod* FWIW, that infoleak _does_ allow to leak an uninitialized word into coredump (in sigreturn the value from uninitialized local variable is copied into pt_regs of process and when we eventually check that error has happened and hit the sucker with SIGSEGV, that value gets stored into the coredump), but in the worst case that's 64 bits leaked from fixed depth in the kernel stack of attacker's process, with fixed call chain. I very much doubt that it's escalatable to anything practically interesting. If spender et.al. can come up with a usable way to escalate that, I would be quite surprised (and would love to see the details), but hey, it might be possible. More likely possibility is that the bug is harmless in practice. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [4.1 backport trouble] Re: BUGreport: fix minor infoleak in get_user_ex() 2016-10-28 19:49 ` Al Viro @ 2016-10-28 21:29 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2016-10-28 21:29 UTC (permalink / raw) To: Al Viro Cc: Linus Torvalds, Joe Korty, Linux Kernel Mailing List, Sasha Levin, stable On Fri, Oct 28, 2016 at 08:49:58PM +0100, Al Viro wrote: > On Fri, Oct 28, 2016 at 11:21:24AM -0700, Linus Torvalds wrote: > > > End result: either commit 1c109fabbd51 shouldn't be backported (it's > > really not that important - if people properly check the exception > > error results it shouldn't matter), or you need to also backport > > 548acf19234d as Al suggested. > > > > I'd be inclined to say "don't backport 1c109fabbd51", but it's really > > a judgment call. > > *nod* > > FWIW, that infoleak _does_ allow to leak an uninitialized word into > coredump (in sigreturn the value from uninitialized local variable is > copied into pt_regs of process and when we eventually check that error > has happened and hit the sucker with SIGSEGV, that value gets stored into > the coredump), but in the worst case that's 64 bits leaked from fixed depth > in the kernel stack of attacker's process, with fixed call chain. > > I very much doubt that it's escalatable to anything practically interesting. > If spender et.al. can come up with a usable way to escalate that, I would be > quite surprised (and would love to see the details), but hey, it might be > possible. More likely possibility is that the bug is harmless in practice. Hm, I think I'll backport 548acf19234d to 4.4-stable, as people have shown that leaking anything can be used in odd ways that they shouldn't be, just to be "safe" :) thanks for the heads up. greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [4.1 backport trouble] Re: BUGreport: fix minor infoleak in get_user_ex() [not found] ` <20161028164033.GA29952@zipoli.ccur.com> 2016-10-28 18:21 ` Linus Torvalds @ 2016-10-28 19:34 ` Al Viro 1 sibling, 0 replies; 7+ messages in thread From: Al Viro @ 2016-10-28 19:34 UTC (permalink / raw) To: Joe Korty; +Cc: linux-kernel, Linus Torvalds, Sasha Levin On Fri, Oct 28, 2016 at 12:40:33PM -0400, Joe Korty wrote: > Backporting 548acf19234d to 4.1.35 does indeed fix the > issue. However, it is not clear to my _why_ it works, > so it might be better that someone else push the backport > to stable. Because the trick used in fixup_exception() prior to that commit depended upon the handler being very close to faulting instruction. # define _ASM_EXTABLE_EX(from,to) \ .pushsection "__ex_table","a" ; \ .balign 8 ; \ .long (from) - . ; \ .long (to) - . + 0x7ffffff0 ; \ .popsection puts a recognizable value (handler + offset a bit under 2G) into ->fixup and in fixup_exception() we had if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) { /* Special hack for uaccess_err */ current_thread_info()->uaccess_err = 1; new_ip -= 0x7ffffff0; } checking that the value in ->fixup is just below 2G from the faulting instruction. So _ASM_EXTABLE_EX relied upon the handler very close to the faulting insn, and worked only because all of its uses had been "set the ->uaccess_err and continue immediately past the faulting insn". When the kludge in fixup_exception() had been eliminated (check what it and _ASM_EXTABLE_EX do these days) this restriction has disappeared, so the mainline commit had no problems. Backport to 4.1 had it run afoul of that restriction, with the results you've observed - this "handler + constant offset" had _not_ been recognized as that magic and had been interpreted as handler being about 2G away from its actual location. That's where the bogus RIP in your oopsen have come from. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [4.1 backport trouble] Re: BUGreport: fix minor infoleak in get_user_ex() 2016-10-28 2:02 ` [4.1 backport trouble] " Al Viro [not found] ` <20161028164033.GA29952@zipoli.ccur.com> @ 2016-10-30 18:16 ` Levin, Alexander 1 sibling, 0 replies; 7+ messages in thread From: Levin, Alexander @ 2016-10-30 18:16 UTC (permalink / raw) To: Al Viro; +Cc: Joe Korty, linux-kernel, Linus Torvalds, Levin, Alexander On Thu, Oct 27, 2016 at 10:02:10PM -0400, Al Viro wrote: > ... and frankly, backporting 548acf19234d would be my preference. It's a bit > more intrusive than needed (_ASM_EXTABLE_FAULT is used only in memcpy_mcsafe(), > which is used only by pmem and it's the only reason for passing the trap > number to fixup_exception()), but AFAICS it's fairly safe. Objections? I've grabbed 548acf19234d for 4.1, thanks! -- Thanks, Sasha ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-10-30 18:26 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20161027193210.GA23006@zipoli.ccur.com> 2016-10-28 0:03 ` BUGreport: fix minor infoleak in get_user_ex() Al Viro 2016-10-28 2:02 ` [4.1 backport trouble] " Al Viro [not found] ` <20161028164033.GA29952@zipoli.ccur.com> 2016-10-28 18:21 ` Linus Torvalds 2016-10-28 19:49 ` Al Viro 2016-10-28 21:29 ` Greg KH 2016-10-28 19:34 ` Al Viro 2016-10-30 18:16 ` Levin, Alexander
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).