KMSAN: kernel-infoleak in copy_siginfo_to_user (2)

[syzbot]

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+0d602a...@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 syzk...@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

[Eric W. Biederman]

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

[Andrew Morton]

On Tue, 28 May 2019 12:34:28 -0500 ebie...@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.

[Eric W. Biederman]

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 <ava...@gmail.com>
Cc: sta...@vger.kernel.org
Reported-by: syzbot+0d602a...@syzkaller.appspotmail.com
Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)")
Signed-off-by: "Eric W. Biederman" <ebie...@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

[Andrei Vagin]

On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebie...@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 <ava...@gmail.com>
> Cc: sta...@vger.kernel.org
> Reported-by: syzbot+0d602a...@syzkaller.appspotmail.com
> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)")
> Signed-off-by: "Eric W. Biederman" <ebie...@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

[Eric W. Biederman]

Andrei Vagin <ava...@gmail.com> writes:

> On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebie...@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 <ava...@gmail.com>
>> Cc: sta...@vger.kernel.org
>> Reported-by: syzbot+0d602a...@syzkaller.appspotmail.com
>> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)")
>> Signed-off-by: "Eric W. Biederman" <ebie...@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

[Eric Biggers]

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