On Tue, Jun 04, 2019 at 02:42:23PM -0500, Eric W. Biederman wrote: > Andrei Vagin <ava...@gmail.com> writes: > > > 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 > > 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 <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) > > > > 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
