Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO

[ebiederm@xmission.com (Eric W. Biederman)]

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