Messages in this thread Patch in this message | | | From | Andrei Vagin <> | Date | Tue, 4 Jun 2019 11:33:06 -0700 | 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@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 >
| |