* KMSAN: kernel-infoleak in copy_siginfo_to_user (2) @ 2019-05-12 10:07 syzbot 2019-05-28 17:34 ` Eric W. Biederman 0 siblings, 1 reply; 7+ messages in thread From: syzbot @ 2019-05-12 10:07 UTC (permalink / raw) To: akpm, arnd, christian, deepa.kernel, ebiederm, glider, linux-kernel, syzkaller-bugs, tglx Hello, syzbot found the following crash on: HEAD commit: d062d017 usb-fuzzer: main usb gadget fuzzer driver git tree: kmsan console output: https://syzkaller.appspot.com/x/log.txt?x=137348b4a00000 kernel config: https://syzkaller.appspot.com/x/.config?x=67ebf8b3cce62ce7 dashboard link: https://syzkaller.appspot.com/bug?extid=0d602a1b0d8c95bdf299 compiler: clang version 9.0.0 (/home/glider/llvm/clang 06d00afa61eef8f7f501ebdb4e8612ea43ec2d78) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=175d65e0a00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ae05c0a00000 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com ptrace attach of "./syz-executor353086472"[10278] was attempted by "./syz-executor353086472"[10279] ptrace attach of "./syz-executor353086472"[10280] was attempted by "./syz-executor353086472"[10281] ptrace attach of "./syz-executor353086472"[10282] was attempted by "./syz-executor353086472"[10283] ================================================================== BUG: KMSAN: kernel-infoleak in _copy_to_user+0x16b/0x1f0 lib/usercopy.c:32 CPU: 1 PID: 10284 Comm: syz-executor353 Not tainted 5.1.0-rc7+ #5 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x191/0x1f0 lib/dump_stack.c:113 kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:619 kmsan_internal_check_memory+0x974/0xa80 mm/kmsan/kmsan.c:713 kmsan_copy_to_user+0xa9/0xb0 mm/kmsan/kmsan_hooks.c:492 _copy_to_user+0x16b/0x1f0 lib/usercopy.c:32 copy_to_user include/linux/uaccess.h:174 [inline] copy_siginfo_to_user+0x80/0x160 kernel/signal.c:3059 ptrace_peek_siginfo kernel/ptrace.c:742 [inline] ptrace_request+0x24bd/0x2950 kernel/ptrace.c:913 arch_ptrace+0x9fa/0x1090 arch/x86/kernel/ptrace.c:868 __do_sys_ptrace kernel/ptrace.c:1155 [inline] __se_sys_ptrace+0x2b9/0x7b0 kernel/ptrace.c:1120 __x64_sys_ptrace+0x56/0x70 kernel/ptrace.c:1120 do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291 entry_SYSCALL_64_after_hwframe+0x63/0xe7 RIP: 0033:0x441cc9 Code: e8 bc e6 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 1b 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00000000007efdd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000065 RAX: ffffffffffffffda RBX: 0000000000000063 RCX: 0000000000441cc9 RDX: 00000000200000c0 RSI: 0000000000000007 RDI: 0000000000004209 RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000716000 R11: 0000000000000246 R12: 0000000000000002 R13: 0000000000402a00 R14: 0000000000000000 R15: 0000000000000000 Local variable description: ----info.i@ptrace_request Variable was created at: ptrace_peek_siginfo kernel/ptrace.c:714 [inline] ptrace_request+0x2161/0x2950 kernel/ptrace.c:913 arch_ptrace+0x9fa/0x1090 arch/x86/kernel/ptrace.c:868 Bytes 0-47 of 48 are uninitialized Memory access of size 48 starts at ffff8880a902fd70 Data copied to user address 0000000000716000 ================================================================== --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see: https://goo.gl/tpsmEJ#testing-patches ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: KMSAN: kernel-infoleak in copy_siginfo_to_user (2) 2019-05-12 10:07 KMSAN: kernel-infoleak in copy_siginfo_to_user (2) syzbot @ 2019-05-28 17:34 ` Eric W. Biederman 2019-05-28 19:47 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: Eric W. Biederman @ 2019-05-28 17:34 UTC (permalink / raw) To: Andrew Morton Cc: arnd, christian, deepa.kernel, glider, linux-kernel, syzkaller-bugs, tglx, Oleg Nesterov, syzbot Andrew, Didn't someone already provide a fix for this one? I thought I saw that hit your tree a while ago. I am looking in ptrace.c and I don't see anything that would have fixed this issue. If there isn't a fix in the queue I will take a stab at it. Thank you Eric syzbot <syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com> writes: > Hello, > > syzbot found the following crash on: > > HEAD commit: d062d017 usb-fuzzer: main usb gadget fuzzer driver > git tree: kmsan > console output: https://syzkaller.appspot.com/x/log.txt?x=137348b4a00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=67ebf8b3cce62ce7 > dashboard link: https://syzkaller.appspot.com/bug?extid=0d602a1b0d8c95bdf299 > compiler: clang version 9.0.0 (/home/glider/llvm/clang > 06d00afa61eef8f7f501ebdb4e8612ea43ec2d78) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=175d65e0a00000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ae05c0a00000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com > > ptrace attach of "./syz-executor353086472"[10278] was attempted by > "./syz-executor353086472"[10279] > ptrace attach of "./syz-executor353086472"[10280] was attempted by > "./syz-executor353086472"[10281] > ptrace attach of "./syz-executor353086472"[10282] was attempted by > "./syz-executor353086472"[10283] > ================================================================== > BUG: KMSAN: kernel-infoleak in _copy_to_user+0x16b/0x1f0 lib/usercopy.c:32 > CPU: 1 PID: 10284 Comm: syz-executor353 Not tainted 5.1.0-rc7+ #5 > Hardware name: Google Google Compute Engine/Google Compute Engine, > BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x191/0x1f0 lib/dump_stack.c:113 > kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:619 > kmsan_internal_check_memory+0x974/0xa80 mm/kmsan/kmsan.c:713 > kmsan_copy_to_user+0xa9/0xb0 mm/kmsan/kmsan_hooks.c:492 > _copy_to_user+0x16b/0x1f0 lib/usercopy.c:32 > copy_to_user include/linux/uaccess.h:174 [inline] > copy_siginfo_to_user+0x80/0x160 kernel/signal.c:3059 > ptrace_peek_siginfo kernel/ptrace.c:742 [inline] > ptrace_request+0x24bd/0x2950 kernel/ptrace.c:913 > arch_ptrace+0x9fa/0x1090 arch/x86/kernel/ptrace.c:868 > __do_sys_ptrace kernel/ptrace.c:1155 [inline] > __se_sys_ptrace+0x2b9/0x7b0 kernel/ptrace.c:1120 > __x64_sys_ptrace+0x56/0x70 kernel/ptrace.c:1120 > do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291 > entry_SYSCALL_64_after_hwframe+0x63/0xe7 > RIP: 0033:0x441cc9 > Code: e8 bc e6 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 > 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d > 01 f0 ff ff 0f 83 1b 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:00000000007efdd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000065 > RAX: ffffffffffffffda RBX: 0000000000000063 RCX: 0000000000441cc9 > RDX: 00000000200000c0 RSI: 0000000000000007 RDI: 0000000000004209 > RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000716000 R11: 0000000000000246 R12: 0000000000000002 > R13: 0000000000402a00 R14: 0000000000000000 R15: 0000000000000000 > > Local variable description: ----info.i@ptrace_request > Variable was created at: > ptrace_peek_siginfo kernel/ptrace.c:714 [inline] > ptrace_request+0x2161/0x2950 kernel/ptrace.c:913 > arch_ptrace+0x9fa/0x1090 arch/x86/kernel/ptrace.c:868 > > Bytes 0-47 of 48 are uninitialized > Memory access of size 48 starts at ffff8880a902fd70 > Data copied to user address 0000000000716000 > ================================================================== > > > --- > This bug is generated by a bot. It may contain errors. > See https://goo.gl/tpsmEJ for more information about syzbot. > syzbot engineers can be reached at syzkaller@googlegroups.com. > > syzbot will keep track of this bug report. See: > https://goo.gl/tpsmEJ#status for how to communicate with syzbot. > syzbot can test patches for this bug, for details see: > https://goo.gl/tpsmEJ#testing-patches ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: KMSAN: kernel-infoleak in copy_siginfo_to_user (2) 2019-05-28 17:34 ` Eric W. Biederman @ 2019-05-28 19:47 ` Andrew Morton 2019-05-29 1:21 ` [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO Eric W. Biederman 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2019-05-28 19:47 UTC (permalink / raw) To: Eric W. Biederman Cc: arnd, christian, deepa.kernel, glider, linux-kernel, syzkaller-bugs, tglx, Oleg Nesterov, syzbot On Tue, 28 May 2019 12:34:28 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote: > Didn't someone already provide a fix for this one? > > I thought I saw that hit your tree a while ago. I am looking in > ptrace.c and I don't see anything that would have fixed this issue. I can't find anything which might have addressed this. > If there isn't a fix in the queue I will take a stab at it. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO 2019-05-28 19:47 ` Andrew Morton @ 2019-05-29 1:21 ` Eric W. Biederman 2019-06-04 18:33 ` Andrei Vagin 0 siblings, 1 reply; 7+ messages in thread From: Eric W. Biederman @ 2019-05-29 1:21 UTC (permalink / raw) To: Andrew Morton Cc: arnd, christian, deepa.kernel, glider, linux-kernel, syzkaller-bugs, tglx, Oleg Nesterov, syzbot, Andrei Vagin Recently syzbot in conjunction with KMSAN reported that ptrace_peek_siginfo can copy an uninitialized siginfo to userspace. Inspecting ptrace_peek_siginfo confirms this. The problem is that off when initialized from args.off can be initialized to a negaive value. At which point the "if (off >= 0)" test to see if off became negative fails because off started off negative. Prevent the core problem by adding a variable found that is only true if a siginfo is found and copied to a temporary in preparation for being copied to userspace. Prevent args.off from being truncated when being assigned to off by testing that off is <= the maximum possible value of off. Convert off to an unsigned long so that we should not have to truncate args.off, we have well defined overflow behavior so if we add another check we won't risk fighting undefined compiler behavior, and so that we have a type whose maximum value is easy to test for. Cc: Andrei Vagin <avagin@gmail.com> Cc: stable@vger.kernel.org Reported-by: syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- Comments? Concerns? Otherwise I will queue this up and send it to Linus. kernel/ptrace.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 6f357f4fc859..4c2b24a885d3 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct *child, if (arg.nr < 0) return -EINVAL; + /* Ensure arg.off fits in an unsigned */ + if (arg.off > ULONG_MAX) + return 0; + if (arg.flags & PTRACE_PEEKSIGINFO_SHARED) pending = &child->signal->shared_pending; else @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct *child, for (i = 0; i < arg.nr; ) { kernel_siginfo_t info; - s32 off = arg.off + i; + unsigned long off = arg.off + i; + bool found = false; spin_lock_irq(&child->sighand->siglock); list_for_each_entry(q, &pending->list, list) { if (!off--) { + found = true; copy_siginfo(&info, &q->info); break; } } spin_unlock_irq(&child->sighand->siglock); - if (off >= 0) /* beyond the end of the list */ + if (!found) /* beyond the end of the list */ break; #ifdef CONFIG_COMPAT -- 2.21.0.dirty ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO 2019-05-29 1:21 ` [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO Eric W. Biederman @ 2019-06-04 18:33 ` Andrei Vagin 2019-06-04 19:42 ` Eric W. Biederman 0 siblings, 1 reply; 7+ messages in thread From: Andrei Vagin @ 2019-06-04 18:33 UTC (permalink / raw) To: Eric W. Biederman, Alexander Potapenko Cc: Andrew Morton, Arnd Bergmann, Christian Brauner, deepa.kernel, LKML, syzkaller-bugs, Thomas Gleixner, Oleg Nesterov, syzbot On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > > Recently syzbot in conjunction with KMSAN reported that > ptrace_peek_siginfo can copy an uninitialized siginfo to userspace. > Inspecting ptrace_peek_siginfo confirms this. > > The problem is that off when initialized from args.off can be > initialized to a negaive value. At which point the "if (off >= 0)" > test to see if off became negative fails because off started off > negative. > > Prevent the core problem by adding a variable found that is only true > if a siginfo is found and copied to a temporary in preparation for > being copied to userspace. > > Prevent args.off from being truncated when being assigned to off by > testing that off is <= the maximum possible value of off. Convert off > to an unsigned long so that we should not have to truncate args.off, > we have well defined overflow behavior so if we add another check we > won't risk fighting undefined compiler behavior, and so that we have a > type whose maximum value is easy to test for. > Hello Eric, Thank you for fixing this issue. Sorry for the late response. I thought it was fixed a few month ago, I remembered that we discussed it: https://lkml.org/lkml/2018/10/10/251 Here are two inline comments. > Cc: Andrei Vagin <avagin@gmail.com> > Cc: stable@vger.kernel.org > Reported-by: syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com > Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > > Comments? > Concerns? > > Otherwise I will queue this up and send it to Linus. > > kernel/ptrace.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > index 6f357f4fc859..4c2b24a885d3 100644 > --- a/kernel/ptrace.c > +++ b/kernel/ptrace.c > @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct *child, > if (arg.nr < 0) > return -EINVAL; > > + /* Ensure arg.off fits in an unsigned */ > + if (arg.off > ULONG_MAX) if (arg.off > ULONG_MAX - arg.nr) > + return 0; maybe we should return EINVAL in this case > + > if (arg.flags & PTRACE_PEEKSIGINFO_SHARED) > pending = &child->signal->shared_pending; > else > @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct *child, > > for (i = 0; i < arg.nr; ) { > kernel_siginfo_t info; > - s32 off = arg.off + i; > + unsigned long off = arg.off + i; > + bool found = false; > > spin_lock_irq(&child->sighand->siglock); > list_for_each_entry(q, &pending->list, list) { > if (!off--) { > + found = true; > copy_siginfo(&info, &q->info); > break; > } > } > spin_unlock_irq(&child->sighand->siglock); > > - if (off >= 0) /* beyond the end of the list */ > + if (!found) /* beyond the end of the list */ > break; > > #ifdef CONFIG_COMPAT > -- > 2.21.0.dirty > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO 2019-06-04 18:33 ` Andrei Vagin @ 2019-06-04 19:42 ` Eric W. Biederman 2019-06-10 19:39 ` Eric Biggers 0 siblings, 1 reply; 7+ messages in thread From: Eric W. Biederman @ 2019-06-04 19:42 UTC (permalink / raw) To: Andrei Vagin Cc: Alexander Potapenko, Andrew Morton, Arnd Bergmann, Christian Brauner, deepa.kernel, LKML, syzkaller-bugs, Thomas Gleixner, Oleg Nesterov, syzbot Andrei Vagin <avagin@gmail.com> writes: > On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> >> Recently syzbot in conjunction with KMSAN reported that >> ptrace_peek_siginfo can copy an uninitialized siginfo to userspace. >> Inspecting ptrace_peek_siginfo confirms this. >> >> The problem is that off when initialized from args.off can be >> initialized to a negaive value. At which point the "if (off >= 0)" >> test to see if off became negative fails because off started off >> negative. >> >> Prevent the core problem by adding a variable found that is only true >> if a siginfo is found and copied to a temporary in preparation for >> being copied to userspace. >> >> Prevent args.off from being truncated when being assigned to off by >> testing that off is <= the maximum possible value of off. Convert off >> to an unsigned long so that we should not have to truncate args.off, >> we have well defined overflow behavior so if we add another check we >> won't risk fighting undefined compiler behavior, and so that we have a >> type whose maximum value is easy to test for. >> > > Hello Eric, > > Thank you for fixing this issue. Sorry for the late response. > I thought it was fixed a few month ago, I remembered that we discussed it: > https://lkml.org/lkml/2018/10/10/251 I was looking for that conversation, and I couldn't find it so I just decided to write a test and fix it. > Here are two inline comments. > > >> Cc: Andrei Vagin <avagin@gmail.com> >> Cc: stable@vger.kernel.org >> Reported-by: syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com >> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)") >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> >> Comments? >> Concerns? >> >> Otherwise I will queue this up and send it to Linus. >> >> kernel/ptrace.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/ptrace.c b/kernel/ptrace.c >> index 6f357f4fc859..4c2b24a885d3 100644 >> --- a/kernel/ptrace.c >> +++ b/kernel/ptrace.c >> @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct *child, >> if (arg.nr < 0) >> return -EINVAL; >> >> + /* Ensure arg.off fits in an unsigned */ >> + if (arg.off > ULONG_MAX) > > if (arg.off > ULONG_MAX - arg.nr) > The new variable found ensures that whatever we pass in we won't return an invalid value. All this test does is guarantee we don't return a much lower entry in the queue. We don't need to take arg.nr into account as we won't try entries that high as the queue will never get that long. The maximum siqueue entries per user is about 2^24. >> + return 0; > > maybe we should return EINVAL in this case But it is a huge request not an invalid request. The request makes perfect sense. For smaller values whose offset is greater than the length of the queue we just return 0 entries found. So I think it makes more sense to just return 0 entries found in this case as well. >> + >> if (arg.flags & PTRACE_PEEKSIGINFO_SHARED) >> pending = &child->signal->shared_pending; >> else >> @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct *child, >> >> for (i = 0; i < arg.nr; ) { >> kernel_siginfo_t info; >> - s32 off = arg.off + i; >> + unsigned long off = arg.off + i; >> + bool found = false; >> >> spin_lock_irq(&child->sighand->siglock); >> list_for_each_entry(q, &pending->list, list) { >> if (!off--) { >> + found = true; >> copy_siginfo(&info, &q->info); >> break; >> } >> } >> spin_unlock_irq(&child->sighand->siglock); >> >> - if (off >= 0) /* beyond the end of the list */ >> + if (!found) /* beyond the end of the list */ >> break; >> >> #ifdef CONFIG_COMPAT >> -- >> 2.21.0.dirty >> Eric ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO 2019-06-04 19:42 ` Eric W. Biederman @ 2019-06-10 19:39 ` Eric Biggers 0 siblings, 0 replies; 7+ messages in thread From: Eric Biggers @ 2019-06-10 19:39 UTC (permalink / raw) To: Eric W. Biederman Cc: Andrei Vagin, Alexander Potapenko, Andrew Morton, Arnd Bergmann, Christian Brauner, deepa.kernel, LKML, syzkaller-bugs, Thomas Gleixner, Oleg Nesterov, syzbot On Tue, Jun 04, 2019 at 02:42:23PM -0500, Eric W. Biederman wrote: > Andrei Vagin <avagin@gmail.com> writes: > > > On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > >> > >> > >> Recently syzbot in conjunction with KMSAN reported that > >> ptrace_peek_siginfo can copy an uninitialized siginfo to userspace. > >> Inspecting ptrace_peek_siginfo confirms this. > >> > >> The problem is that off when initialized from args.off can be > >> initialized to a negaive value. At which point the "if (off >= 0)" > >> test to see if off became negative fails because off started off > >> negative. > >> > >> Prevent the core problem by adding a variable found that is only true > >> if a siginfo is found and copied to a temporary in preparation for > >> being copied to userspace. > >> > >> Prevent args.off from being truncated when being assigned to off by > >> testing that off is <= the maximum possible value of off. Convert off > >> to an unsigned long so that we should not have to truncate args.off, > >> we have well defined overflow behavior so if we add another check we > >> won't risk fighting undefined compiler behavior, and so that we have a > >> type whose maximum value is easy to test for. > >> > > > > Hello Eric, > > > > Thank you for fixing this issue. Sorry for the late response. > > I thought it was fixed a few month ago, I remembered that we discussed it: > > https://lkml.org/lkml/2018/10/10/251 > > I was looking for that conversation, and I couldn't find it so I just > decided to write a test and fix it. > > > Here are two inline comments. > > > > > >> Cc: Andrei Vagin <avagin@gmail.com> > >> Cc: stable@vger.kernel.org > >> Reported-by: syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com > >> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)") > >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > >> --- > >> > >> Comments? > >> Concerns? > >> > >> Otherwise I will queue this up and send it to Linus. > >> > >> kernel/ptrace.c | 10 ++++++++-- > >> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/kernel/ptrace.c b/kernel/ptrace.c > >> index 6f357f4fc859..4c2b24a885d3 100644 > >> --- a/kernel/ptrace.c > >> +++ b/kernel/ptrace.c > >> @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct *child, > >> if (arg.nr < 0) > >> return -EINVAL; > >> > >> + /* Ensure arg.off fits in an unsigned */ > >> + if (arg.off > ULONG_MAX) > > > > if (arg.off > ULONG_MAX - arg.nr) > > > > The new variable found ensures that whatever we pass in we won't return > an invalid value. All this test does is guarantee we don't return a > much lower entry in the queue. > > We don't need to take arg.nr into account as we won't try > entries that high as the queue will never get that long. The maximum > siqueue entries per user is about 2^24. > > >> + return 0; > > > > maybe we should return EINVAL in this case > > But it is a huge request not an invalid request. The request > makes perfect sense. For smaller values whose offset is > greater than the length of the queue we just return 0 entries > found. So I think it makes more sense to just return 0 entries > found in this case as well. > > >> + > >> if (arg.flags & PTRACE_PEEKSIGINFO_SHARED) > >> pending = &child->signal->shared_pending; > >> else > >> @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct *child, > >> > >> for (i = 0; i < arg.nr; ) { > >> kernel_siginfo_t info; > >> - s32 off = arg.off + i; > >> + unsigned long off = arg.off + i; > >> + bool found = false; > >> > >> spin_lock_irq(&child->sighand->siglock); > >> list_for_each_entry(q, &pending->list, list) { > >> if (!off--) { > >> + found = true; > >> copy_siginfo(&info, &q->info); > >> break; > >> } > >> } > >> spin_unlock_irq(&child->sighand->siglock); > >> > >> - if (off >= 0) /* beyond the end of the list */ > >> + if (!found) /* beyond the end of the list */ > >> break; > >> > >> #ifdef CONFIG_COMPAT > >> -- > >> 2.21.0.dirty > >> > This patch looks fine to me. Are you planning to queue this up? It would be nice if we could fix this sort of bug in fewer than 8 months. - Eric ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-10 19:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-12 10:07 KMSAN: kernel-infoleak in copy_siginfo_to_user (2) syzbot 2019-05-28 17:34 ` Eric W. Biederman 2019-05-28 19:47 ` Andrew Morton 2019-05-29 1:21 ` [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO Eric W. Biederman 2019-06-04 18:33 ` Andrei Vagin 2019-06-04 19:42 ` Eric W. Biederman 2019-06-10 19:39 ` Eric Biggers
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).