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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANaxB-ztx3-3cfsbK4rTnGAAcODJmgKHyhHF_0oBe+qqyf5Leg@mail.gmail.com>
Date:   Tue, 4 Jun 2019 11:33:06 -0700
From:   Andrei Vagin <avagin@...il.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Alexander Potapenko <glider@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Christian Brauner <christian@...uner.io>,
        deepa.kernel@...il.com, LKML <linux-kernel@...r.kernel.org>,
        syzkaller-bugs@...glegroups.com,
        Thomas Gleixner <tglx@...utronix.de>,
        Oleg Nesterov <oleg@...hat.com>,
        syzbot <syzbot+0d602a1b0d8c95bdf299@...kaller.appspotmail.com>
Subject: Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO

On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebiederm@...ssion.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@...il.com>
> Cc: stable@...r.kernel.org
> Reported-by: syzbot+0d602a1b0d8c95bdf299@...kaller.appspotmail.com
> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)")
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.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
>