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

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Jun 04, 2019 at 02:42:23PM -0500, Eric W. Biederman wrote:
> 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
> >>
> 

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