Received: by 2002:a25:ab43:0:0:0:0:0 with SMTP id u61csp3576332ybi; Mon, 10 Jun 2019 12:39:43 -0700 (PDT) X-Google-Smtp-Source: APXvYqxAkFEQy8WZtkYcGzlhtt97785GwaVk0ZUcG0qFyV/yNoE2jzOH42VALZoCmOgTNYFfA1f1 X-Received: by 2002:aa7:8555:: with SMTP id y21mr19386150pfn.104.1560195583251; Mon, 10 Jun 2019 12:39:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1560195583; cv=none; d=google.com; s=arc-20160816; b=0nq5L6mFtDXlr0++sjcCNnZwoEhfuKkY/PoX7U1oBxpvf1z4V0i9rmResLZX3IgVlF Yn17HpyL4/bi/znJoeCrvDEWUWNHhhhWus0hsliXxeSQYZ9hPKB4pg091ydXwO9V69Kb fctEz1P+/8mPeM2iIKRJLyaRExRk+G0+ILVDfh+zpraHps2M8Os50QBZqEPfAq8chNOw 3ClTdlzVIybIg6xl0kwo4pSx7blO8YJzHb95+apwuKN6GqM1xWBbb7uQtaCxKnJ/X+4Q j7a4UWp+VF56LlOqGlOwOA96FdsQ4bvaBrKiunfYM+lGYzeBTScCT1k3aJLUwqYKzIRs WLLQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=VKIrWluz265As/SPuoINkfQa0RrbIV54HRuLsaz6EbA=; b=Ts+hY1fnKK57pFjUXIqy7JBT25CK04AiHPw2pXxCnu1o/wztL0HcM9BJTuygOnDeJy D6W412rkP4QKfUIO71zfjQGEmZNblj0L6owP+QTjptxf45BScVYPKCyGO610MTIpQpwq wZg0RPCtcX8eABpYkgs0o7FN+P4HHIsMC1vVk86qYUIIlDZLbqJvFqg1nd4CAWb4qh+W DFveNFt39ZLas1r7cXmRlUcFx1xPbwDgXPLZCOi/pZk5hN2po20WgQSQDb5Xqvh1RQcn Y5JNtkQvAnBR1dL6+rXOsU2nCu3rBqIQP430aP/4O3AjUqYBui5DcfnZu/C+MzU7Is9v msFQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="O65CO/Tc"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id l8si263673pjq.56.2019.06.10.12.39.27; Mon, 10 Jun 2019 12:39:43 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="O65CO/Tc"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389099AbfFJTjX (ORCPT + 99 others); Mon, 10 Jun 2019 15:39:23 -0400 Received: from mail.kernel.org ([198.145.29.99]:52948 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388843AbfFJTjW (ORCPT ); Mon, 10 Jun 2019 15:39:22 -0400 Received: from gmail.com (unknown [104.132.1.77]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 0130B207E0; Mon, 10 Jun 2019 19:39:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1560195561; bh=Yc63F9HkWkaur2zmRzaHvYzUSbn6hzEs6D1ov72qWGw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=O65CO/TcJ5/PiPp67uiEOxDsBdkHzwiB79Q7AqgvRHktKbX5e4y7ZNwY+W1cWBBpf dA0eQQ8sqSpmU8TJEm5aSULsgHSz4gjh2h7m1XZ5Dz7gSpiksUYs43a9PvHRlJ5I4m eXu3Bxe1cUhNpRURnt3PUZ441wh07BSCWL695nAg= Date: Mon, 10 Jun 2019 12:39:19 -0700 From: Eric Biggers To: "Eric W. Biederman" Cc: Andrei Vagin , Alexander Potapenko , Andrew Morton , Arnd Bergmann , Christian Brauner , deepa.kernel@gmail.com, LKML , syzkaller-bugs@googlegroups.com, Thomas Gleixner , Oleg Nesterov , syzbot Subject: Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO Message-ID: <20190610193918.GJ63833@gmail.com> References: <000000000000410d500588adf637@google.com> <87woia5vq3.fsf@xmission.com> <20190528124746.ac703cd668ca9409bb79100b@linux-foundation.org> <87pno23vim.fsf_-_@xmission.com> <87tvd5m928.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87tvd5m928.fsf@xmission.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 04, 2019 at 02:42:23PM -0500, Eric W. Biederman wrote: > Andrei Vagin writes: > > > On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman 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 > >> 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" > >> --- > >> > >> 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