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

[Andrei Vagin]

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