Re: [PATCH 4/5] MIPS: Execute any partial write of the last register with PTRACE_SETREGSET

From: Dave Martin <Dave.Martin@arm.com>
To: "Maciej W. Rozycki" <macro@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>,
	James Hogan <james.hogan@mips.com>,
	Paul Burton <Paul.Burton@mips.com>,
	Alex Smith <alex@alex-smith.me.uk>,
	"linux-mips@linux-mips.org" <linux-mips@linux-mips.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH 4/5] MIPS: Execute any partial write of the last register with PTRACE_SETREGSET
Date: Fri, 1 Dec 2017 12:35:25 +0000	[thread overview]
Message-ID: <20171201123525.GS22781@e103592.cambridge.arm.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1711301831540.31156@tp.orcam.me.uk>

From: Dave Martin <Dave.Martin@arm.com>
To: "Maciej W. Rozycki" <macro@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>, James Hogan <james.hogan@mips.com>,
Subject: Re: [PATCH 4/5] MIPS: Execute any partial write of the last register
 with PTRACE_SETREGSET
In-Reply-To: <alpine.DEB.2.00.1711301831540.31156@tp.orcam.me.uk>

On Thu, Nov 30, 2017 at 07:38:25PM +0000, Maciej W. Rozycki wrote:

[...]

> > If we can end up with that somehow, then this patch reintroduces the
> > issue d614fd58a283 aims to fix, whereby fpr_val can contain
> > uninitialised kernel stack which userspace can then obtain via
> > PTRACE_GETREGSET.
> 
>  That wasn't actually clarified in the referred commit's description, 
> which it should in the first place, and I wasn't able to track down any 
> review of your change as submitted, which would be the potential second 
> source of such support information.  The description isn't even correct, 
> as it states that if a short buffer is supplied, then the old values held 
> in thread's registers are preserved, which clearly isn't correct as 
> individual registers do get written from the beginning of the regset up to 
> the point no more data is available to fill a whole register.

FYI, this this patch wasn't discussed on the public lists because of the
security implications of kernel stack exposure.  IIRC, James and Ralf
were Cc'd on the discussion.

There's an awkward balance to be struck between giving a full
description of the change and the motivation for it, and avoiding
announcing publicly exactly how to exploit the bug.  Opinions differ on
where the correct balance lies.

So, the commit message was intentionally kept vague, but could have
been better in this instance, since it doesn't correctly describe the
change as committed.

Cheers
---Dave