On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebied...@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 <ava...@gmail.com> > Cc: sta...@vger.kernel.org > Reported-by: syzbot+0d602a1b0d8c95bdf...@syzkaller.appspotmail.com > Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without > removing from a queue (v4)") > Signed-off-by: "Eric W. Biederman" <ebied...@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 >
