Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <000000000000410d500588adf637@google.com> <87woia5vq3.fsf@xmission.com>
 <20190528124746.ac703cd668ca9409bb79100b@linux-foundation.org> <87pno23vim.fsf_-_@xmission.com>
In-Reply-To: <87pno23vim.fsf_-_@xmission.com>
From:   Andrei Vagin <avagin@gmail.com>
Date:   Tue, 4 Jun 2019 11:33:06 -0700
Message-ID: <CANaxB-ztx3-3cfsbK4rTnGAAcODJmgKHyhHF_0oBe+qqyf5Leg@mail.gmail.com>
Subject: Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO
To:     "Eric W. Biederman" <ebiederm@xmission.com>,
        Alexander Potapenko <glider@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Christian Brauner <christian@brauner.io>,
        deepa.kernel@gmail.com, LKML <linux-kernel@vger.kernel.org>,
        syzkaller-bugs@googlegroups.com,
        Thomas Gleixner <tglx@linutronix.de>,
        Oleg Nesterov <oleg@redhat.com>,
        syzbot <syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>