[PATCH 0/5] MIPS: NT_PRFPREG regset handling fixes

[PATCH 0/5] MIPS: NT_PRFPREG regset handling fixes

[Maciej W. Rozycki]

Hi,

 This series corrects a number of issues with NT_PRFPREG regset, most 
importantly an FCSR access API regression introduced with the addition of 
MSA support, and then a few smaller issues with the get/set handlers.

 I have decided to factor out non-MSA and MSA context helpers as the first 
step to avoid the issue with excessive indentation that would inevitably 
happen if the regression fix was applied to current code as it stands.  
It shouldn't be a big deal with backporting as this code hasn't changed 
much since the regression, and it will make any future bacports easier.  
Only a call to `init_fp_ctx' will have to be trivially resolved (though 
arguably commit ac9ad83bc318 ("MIPS: prevent FP context set via ptrace 
being discarded"), which has added `init_fp_ctx', would be good to 
backport as far as possible instead).

 These changes have been verified by examining the register state recorded 
in core dumps manually with GDB, as well as by running the GDB test suite.  
No user of ptrace(2) PTRACE_GETREGSET and PTRACE_SETREGSET requests is 
known for the MIPS port, so this part remains not covered, however it is 
assumed to remain consistent with how the creation of core file works.

 See individual patch descriptions for further details.

  Maciej

[PATCH 1/5] MIPS: Factor out NT_PRFPREG regset access helpers

[Maciej W. Rozycki]

In preparation to fix a commit 72b22bbad1e7 ("MIPS: Don't assume 64-bit 
FP registers for FP regset") FCSR access regression factor out 
NT_PRFPREG regset access helpers for the non-MSA and the MSA variants 
respectively, to avoid having to deal with excessive indentation in the
actual fix.

No functional change, however use `target->thread.fpu.fpr[0]' rather 
than `target->thread.fpu.fpr[i]' for FGR holding type size determination 
as there's no `i' variable to refer to anymore, and for the factored out 
`i' variable declaration use `unsigned int' rather than `unsigned' as 
its type, following the common style.

Cc: stable@vger.kernel.org # v3.15+
Fixes: 72b22bbad1e7 ("MIPS: Don't assume 64-bit FP registers for FP regset")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
 arch/mips/kernel/ptrace.c |  108 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 83 insertions(+), 25 deletions(-)

linux-mips-nt-prfpreg-factor-out.diff
Index: linux-sfr-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-23 19:18:08.831530000 +0000
+++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-28 22:44:11.537151000 +0000
@@ -410,25 +410,36 @@ static int gpr64_set(struct task_struct 
 
 #endif /* CONFIG_64BIT */
 
-static int fpr_get(struct task_struct *target,
-		   const struct user_regset *regset,
-		   unsigned int pos, unsigned int count,
-		   void *kbuf, void __user *ubuf)
+/*
+ * Copy the floating-point context to the supplied NT_PRFPREG buffer,
+ * !CONFIG_CPU_HAS_MSA variant.  FP context's general register slots
+ * correspond 1:1 to buffer slots.
+ */
+static int fpr_get_fpa(struct task_struct *target,
+		       unsigned int *pos, unsigned int *count,
+		       void **kbuf, void __user **ubuf)
 {
-	unsigned i;
-	int err;
-	u64 fpr_val;
-
-	/* XXX fcr31  */
+	return user_regset_copyout(pos, count, kbuf, ubuf,
+				   &target->thread.fpu,
+				   0, sizeof(elf_fpregset_t));
+}
 
-	if (sizeof(target->thread.fpu.fpr[i]) == sizeof(elf_fpreg_t))
-		return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
-					   &target->thread.fpu,
-					   0, sizeof(elf_fpregset_t));
+/*
+ * Copy the floating-point context to the supplied NT_PRFPREG buffer,
+ * CONFIG_CPU_HAS_MSA variant.  Only lower 64 bits of FP context's
+ * general register slots are copied to buffer slots.
+ */
+static int fpr_get_msa(struct task_struct *target,
+		       unsigned int *pos, unsigned int *count,
+		       void **kbuf, void __user **ubuf)
+{
+	unsigned int i;
+	u64 fpr_val;
+	int err;
 
 	for (i = 0; i < NUM_FPU_REGS; i++) {
 		fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
-		err = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+		err = user_regset_copyout(pos, count, kbuf, ubuf,
 					  &fpr_val, i * sizeof(elf_fpreg_t),
 					  (i + 1) * sizeof(elf_fpreg_t));
 		if (err)
@@ -438,27 +449,54 @@ static int fpr_get(struct task_struct *t
 	return 0;
 }
 
-static int fpr_set(struct task_struct *target,
+/* Copy the floating-point context to the supplied NT_PRFPREG buffer.  */
+static int fpr_get(struct task_struct *target,
 		   const struct user_regset *regset,
 		   unsigned int pos, unsigned int count,
-		   const void *kbuf, const void __user *ubuf)
+		   void *kbuf, void __user *ubuf)
 {
-	unsigned i;
 	int err;
-	u64 fpr_val;
 
 	/* XXX fcr31  */
 
-	init_fp_ctx(target);
+	if (sizeof(target->thread.fpu.fpr[0]) == sizeof(elf_fpreg_t))
+		err = fpr_get_fpa(target, &pos, &count, &kbuf, &ubuf);
+	else
+		err = fpr_get_msa(target, &pos, &count, &kbuf, &ubuf);
 
-	if (sizeof(target->thread.fpu.fpr[i]) == sizeof(elf_fpreg_t))
-		return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-					  &target->thread.fpu,
-					  0, sizeof(elf_fpregset_t));
+	return err;
+}
+
+/*
+ * Copy the supplied NT_PRFPREG buffer to the floating-point context,
+ * !CONFIG_CPU_HAS_MSA variant.   Buffer slots correspond 1:1 to FP
+ * context's general register slots.
+ */
+static int fpr_set_fpa(struct task_struct *target,
+		       unsigned int *pos, unsigned int *count,
+		       const void **kbuf, const void __user **ubuf)
+{
+	return user_regset_copyin(pos, count, kbuf, ubuf,
+				  &target->thread.fpu,
+				  0, sizeof(elf_fpregset_t));
+}
+
+/*
+ * Copy the supplied NT_PRFPREG buffer to the floating-point context,
+ * CONFIG_CPU_HAS_MSA variant.  Buffer slots are copied to lower 64
+ * bits only of FP context's general register slots.
+ */
+static int fpr_set_msa(struct task_struct *target,
+		       unsigned int *pos, unsigned int *count,
+		       const void **kbuf, const void __user **ubuf)
+{
+	unsigned int i;
+	u64 fpr_val;
+	int err;
 
 	BUILD_BUG_ON(sizeof(fpr_val) != sizeof(elf_fpreg_t));
-	for (i = 0; i < NUM_FPU_REGS && count >= sizeof(elf_fpreg_t); i++) {
-		err = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+	for (i = 0; i < NUM_FPU_REGS && *count >= sizeof(elf_fpreg_t); i++) {
+		err = user_regset_copyin(pos, count, kbuf, ubuf,
 					 &fpr_val, i * sizeof(elf_fpreg_t),
 					 (i + 1) * sizeof(elf_fpreg_t));
 		if (err)
@@ -469,6 +507,26 @@ static int fpr_set(struct task_struct *t
 	return 0;
 }
 
+/* Copy the supplied NT_PRFPREG buffer to the floating-point context.  */
+static int fpr_set(struct task_struct *target,
+		   const struct user_regset *regset,
+		   unsigned int pos, unsigned int count,
+		   const void *kbuf, const void __user *ubuf)
+{
+	int err;
+
+	/* XXX fcr31  */
+
+	init_fp_ctx(target);
+
+	if (sizeof(target->thread.fpu.fpr[0]) == sizeof(elf_fpreg_t))
+		err = fpr_set_fpa(target, &pos, &count, &kbuf, &ubuf);
+	else
+		err = fpr_set_msa(target, &pos, &count, &kbuf, &ubuf);
+
+	return err;
+}
+
 enum mips_regset {
 	REGSET_GPR,
 	REGSET_FPR,

[PATCH 2/5] MIPS: Fix an FCSR access API regression with NT_PRFPREG and MSA

[Maciej W. Rozycki]

Fix a commit 72b22bbad1e7 ("MIPS: Don't assume 64-bit FP registers for 
FP regset") public API regression, then activated by commit 1db1af84d6df 
("MIPS: Basic MSA context switching support"), that caused the FCSR 
register not to be read or written for CONFIG_CPU_HAS_MSA kernel 
configurations (regardless of actual presence or absence of the MSA 
feature in a given processor) with ptrace(2) PTRACE_GETREGSET and 
PTRACE_SETREGSET requests nor recorded in core dumps.

This is because with !CONFIG_CPU_HAS_MSA configurations the whole of 
`elf_fpregset_t' array is bulk-copied as it is, which includes the FCSR 
in one half of the last, 33rd slot, whereas with CONFIG_CPU_HAS_MSA 
configurations array elements are copied individually, and then only the 
leading 32 FGR slots while the remaining slot is ignored.

Correct the code then such that only FGR slots are copied in the 
respective !MSA and MSA helpers an then the FCSR slot is handled 
separately in common code.  Use `ptrace_setfcr31' to update the FCSR 
too, so that the read-only mask is respected.

Retrieving a correct value of FCSR is important in debugging not only 
for the human to be able to get the right interpretation of the 
situation, but for correct operation of GDB as well.  This is because 
the condition code bits in FSCR are used by GDB to determine the 
location to place a breakpoint at when single-stepping through an FPU 
branch instruction.  If such a breakpoint is placed incorrectly (i.e. 
with the condition reversed), then it will be missed, likely causing the 
debuggee to run away from the control of GDB and consequently breaking 
the process of investigation.

Fortunately GDB continues using the older PTRACE_GETFPREGS ptrace(2) 
request which is unaffected, so the regression only really hits with 
post-mortem debug sessions using a core dump file, in which case 
execution, and consequently single-stepping through branches is not 
possible.  Of course core files created by buggy kernels out there will 
have the value of FCSR recorded clobbered, but such core files cannot be 
corrected and the person using them simply will have to be aware that 
the value of FCSR retrieved is not reliable.

Which also means we can likely get away without defining a replacement 
API which would ensure a correct value of FSCR to be retrieved, or none 
at all.

This is based on previous work by Alex Smith, extensively rewritten.

Cc: stable@vger.kernel.org # v3.15+
Fixes: 72b22bbad1e7 ("MIPS: Don't assume 64-bit FP registers for FP regset")
Signed-off-by: Alex Smith <alex@alex-smith.me.uk>
Signed-off-by: James Hogan <james.hogan@mips.com>
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---

 I think the fix to use `ptrace_setfcr31' could be a separate change, but 
given how its absence has been entangled with the inconsistency between
!MSA and MSA I cannot imagine how to do it in a sane manner:

1. If done first, then it would have to be applied to the !MSA case only,
   only to be moved to shared code in the next step.

2. If done second, then new incorrect shared code would have to be written
   only to be fixed in the next step.

So I think it has to go along with the main fix.  Though maybe choosing #2 
would make backporting easier, as `ptrace_setfcr31' is v4.1+ only and:

		target->thread.fpu.fcr31 = fcr31;

has to be used before that (previously the read-only mask wasn't defined
and you could write arbitrary rubbish to FCSR, especially in the emulated
case).

 Thoughts?

  Maciej

---
 arch/mips/kernel/ptrace.c |   51 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 12 deletions(-)

linux-mips-nt-prfpreg-fcsr.diff
Index: linux-sfr-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-28 22:44:11.000000000 +0000
+++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-28 23:33:33.395023000 +0000
@@ -413,7 +413,7 @@ static int gpr64_set(struct task_struct 
 /*
  * Copy the floating-point context to the supplied NT_PRFPREG buffer,
  * !CONFIG_CPU_HAS_MSA variant.  FP context's general register slots
- * correspond 1:1 to buffer slots.
+ * correspond 1:1 to buffer slots.  Only general registers are copied.
  */
 static int fpr_get_fpa(struct task_struct *target,
 		       unsigned int *pos, unsigned int *count,
@@ -421,13 +421,14 @@ static int fpr_get_fpa(struct task_struc
 {
 	return user_regset_copyout(pos, count, kbuf, ubuf,
 				   &target->thread.fpu,
-				   0, sizeof(elf_fpregset_t));
+				   0, NUM_FPU_REGS * sizeof(elf_fpreg_t));
 }
 
 /*
  * Copy the floating-point context to the supplied NT_PRFPREG buffer,
  * CONFIG_CPU_HAS_MSA variant.  Only lower 64 bits of FP context's
- * general register slots are copied to buffer slots.
+ * general register slots are copied to buffer slots.  Only general
+ * registers are copied.
  */
 static int fpr_get_msa(struct task_struct *target,
 		       unsigned int *pos, unsigned int *count,
@@ -449,20 +450,29 @@ static int fpr_get_msa(struct task_struc
 	return 0;
 }
 
-/* Copy the floating-point context to the supplied NT_PRFPREG buffer.  */
+/*
+ * Copy the floating-point context to the supplied NT_PRFPREG buffer.
+ * Choose the appropriate helper for general registers, and then copy
+ * the FCSR register separately.
+ */
 static int fpr_get(struct task_struct *target,
 		   const struct user_regset *regset,
 		   unsigned int pos, unsigned int count,
 		   void *kbuf, void __user *ubuf)
 {
+	const int fcr31_pos = NUM_FPU_REGS * sizeof(elf_fpreg_t);
 	int err;
 
-	/* XXX fcr31  */
-
 	if (sizeof(target->thread.fpu.fpr[0]) == sizeof(elf_fpreg_t))
 		err = fpr_get_fpa(target, &pos, &count, &kbuf, &ubuf);
 	else
 		err = fpr_get_msa(target, &pos, &count, &kbuf, &ubuf);
+	if (err)
+		return err;
+
+	err = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+				  &target->thread.fpu.fcr31,
+				  fcr31_pos, fcr31_pos + sizeof(u32));
 
 	return err;
 }
@@ -470,7 +480,7 @@ static int fpr_get(struct task_struct *t
 /*
  * Copy the supplied NT_PRFPREG buffer to the floating-point context,
  * !CONFIG_CPU_HAS_MSA variant.   Buffer slots correspond 1:1 to FP
- * context's general register slots.
+ * context's general register slots.  Only general registers are copied.
  */
 static int fpr_set_fpa(struct task_struct *target,
 		       unsigned int *pos, unsigned int *count,
@@ -478,13 +488,14 @@ static int fpr_set_fpa(struct task_struc
 {
 	return user_regset_copyin(pos, count, kbuf, ubuf,
 				  &target->thread.fpu,
-				  0, sizeof(elf_fpregset_t));
+				  0, NUM_FPU_REGS * sizeof(elf_fpreg_t));
 }
 
 /*
  * Copy the supplied NT_PRFPREG buffer to the floating-point context,
  * CONFIG_CPU_HAS_MSA variant.  Buffer slots are copied to lower 64
- * bits only of FP context's general register slots.
+ * bits only of FP context's general register slots.  Only general
+ * registers are copied.
  */
 static int fpr_set_msa(struct task_struct *target,
 		       unsigned int *pos, unsigned int *count,
@@ -507,22 +518,38 @@ static int fpr_set_msa(struct task_struc
 	return 0;
 }
 
-/* Copy the supplied NT_PRFPREG buffer to the floating-point context.  */
+/*
+ * Copy the supplied NT_PRFPREG buffer to the floating-point context.
+ * Choose the appropriate helper for general registers, and then copy
+ * the FCSR register separately.
+ */
 static int fpr_set(struct task_struct *target,
 		   const struct user_regset *regset,
 		   unsigned int pos, unsigned int count,
 		   const void *kbuf, const void __user *ubuf)
 {
+	const int fcr31_pos = NUM_FPU_REGS * sizeof(elf_fpreg_t);
+	u32 fcr31;
 	int err;
 
-	/* XXX fcr31  */
-
 	init_fp_ctx(target);
 
 	if (sizeof(target->thread.fpu.fpr[0]) == sizeof(elf_fpreg_t))
 		err = fpr_set_fpa(target, &pos, &count, &kbuf, &ubuf);
 	else
 		err = fpr_set_msa(target, &pos, &count, &kbuf, &ubuf);
+	if (err)
+		return err;
+
+	if (count > 0) {
+		err = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+					 &fcr31,
+					 fcr31_pos, fcr31_pos + sizeof(u32));
+		if (err)
+			return err;
+
+		ptrace_setfcr31(target, fcr31);
+	}
 
 	return err;
 }

[PATCH 3/5] MIPS: Also verify sizeof `elf_fpreg_t' with PTRACE_SETREGSET

[Maciej W. Rozycki]

Complement commit d614fd58a283 ("mips/ptrace: Preserve previous 
registers for short regset write") and like with the PTRACE_GETREGSET 
ptrace(2) request also apply a BUILD_BUG_ON check for the size of the 
`elf_fpreg_t' type in the PTRACE_SETREGSET request handler.

Cc: stable@vger.kernel.org # v4.11+
Fixes: d614fd58a283 ("mips/ptrace: Preserve previous registers for short regset write")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
 arch/mips/kernel/ptrace.c |    1 +
 1 file changed, 1 insertion(+)

linux-mips-nt-prfpreg-build-bug.diff
Index: linux-sfr-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-28 23:33:33.395023000 +0000
+++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-28 23:52:34.944549000 +0000
@@ -438,6 +438,7 @@ static int fpr_get_msa(struct task_struc
 	u64 fpr_val;
 	int err;
 
+	BUILD_BUG_ON(sizeof(fpr_val) != sizeof(elf_fpreg_t));
 	for (i = 0; i < NUM_FPU_REGS; i++) {
 		fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
 		err = user_regset_copyout(pos, count, kbuf, ubuf,

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

[Maciej W. Rozycki]

Fix a commit d614fd58a283 ("mips/ptrace: Preserve previous registers for 
short regset write") bug and allow the last register requested with a 
ptrace(2) PTRACE_SETREGSET call to be partially written if supplied this 
way by the caller, like with other register sets.

Cc: stable@vger.kernel.org # v4.11+
Fixes: d614fd58a283 ("mips/ptrace: Preserve previous registers for short regset write")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
 arch/mips/kernel/ptrace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

linux-mips-nt-prfpreg-count.diff
Index: linux-sfr-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-21 22:12:00.000000000 +0000
+++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-21 22:13:13.471970000 +0000
@@ -484,7 +484,7 @@ static int fpr_set_msa(struct task_struc
 	int err;
 
 	BUILD_BUG_ON(sizeof(fpr_val) != sizeof(elf_fpreg_t));
-	for (i = 0; i < NUM_FPU_REGS && *count >= sizeof(elf_fpreg_t); i++) {
+	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
 		err = user_regset_copyin(pos, count, kbuf, ubuf,
 					 &fpr_val, i * sizeof(elf_fpreg_t),
 					 (i + 1) * sizeof(elf_fpreg_t));

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

[Dave Martin]

On Wed, Nov 29, 2017 at 03:21:14PM +0000, Maciej W. Rozycki wrote:
> Fix a commit d614fd58a283 ("mips/ptrace: Preserve previous registers for 
> short regset write") bug and allow the last register requested with a 
> ptrace(2) PTRACE_SETREGSET call to be partially written if supplied this 
> way by the caller, like with other register sets.
> 
> Cc: stable@vger.kernel.org # v4.11+
> Fixes: d614fd58a283 ("mips/ptrace: Preserve previous registers for short regset write")
> Signed-off-by: Maciej W. Rozycki <macro@mips.com>
> ---
>  arch/mips/kernel/ptrace.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> linux-mips-nt-prfpreg-count.diff
> Index: linux-sfr-test/arch/mips/kernel/ptrace.c
> ===================================================================
> --- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-21 22:12:00.000000000 +0000
> +++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-21 22:13:13.471970000 +0000
> @@ -484,7 +484,7 @@ static int fpr_set_msa(struct task_struc
>  	int err;
>  
>  	BUILD_BUG_ON(sizeof(fpr_val) != sizeof(elf_fpreg_t));
> -	for (i = 0; i < NUM_FPU_REGS && *count >= sizeof(elf_fpreg_t); i++) {
> +	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> 
>  		err = user_regset_copyin(pos, count, kbuf, ubuf,
>  					 &fpr_val, i * sizeof(elf_fpreg_t),
>  					 (i + 1) * sizeof(elf_fpreg_t));

But mips*_regsets[REGSET_FPR].size == sizeof(elf_fpreg_t),
linux/kernel/regset.c:ptrace_regset() polices
iov_len % regset->size == 0, and each user_regset_copyout() call here
transfers sizeof(elf_fpreg_t) bytes, decrementing *count by that
amount unless something goest wrong in which case we return.

So how do we end up with *count > 0 && *count < sizeof(elf_fpreg_t)
here?

If we can't end up with that, then this patch doesn't change ABI-
observable behaviour, unless I've missed something.

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.

Cheers
---Dave

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

[Maciej W. Rozycki]

Hi Dave,

> > linux-mips-nt-prfpreg-count.diff
> > Index: linux-sfr-test/arch/mips/kernel/ptrace.c
> > ===================================================================
> > --- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-21 22:12:00.000000000 +0000
> > +++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-21 22:13:13.471970000 +0000
> > @@ -484,7 +484,7 @@ static int fpr_set_msa(struct task_struc
> >  	int err;
> >  
> >  	BUILD_BUG_ON(sizeof(fpr_val) != sizeof(elf_fpreg_t));
> > -	for (i = 0; i < NUM_FPU_REGS && *count >= sizeof(elf_fpreg_t); i++) {
> > +	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > 
> >  		err = user_regset_copyin(pos, count, kbuf, ubuf,
> >  					 &fpr_val, i * sizeof(elf_fpreg_t),
> >  					 (i + 1) * sizeof(elf_fpreg_t));
> 
> But mips*_regsets[REGSET_FPR].size == sizeof(elf_fpreg_t),
> linux/kernel/regset.c:ptrace_regset() polices
> iov_len % regset->size == 0, and each user_regset_copyout() call here
> transfers sizeof(elf_fpreg_t) bytes, decrementing *count by that
> amount unless something goest wrong in which case we return.

 Good point, I missed that check.

 I don't think however that re-enforcing in arch code, especially in such 
a subtle way, a constraint that has already been enforced upstream in 
generic code is a good idea, because if we ever decide to relax the 
constraint, then all the arch code will have to be carefully reviewed.

> If we can't end up with that, then this patch doesn't change ABI-
> observable behaviour, unless I've missed something.

 Right, in which case there is no need to backport this change if it is 
given a go-ahead.

> 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.

 You are of course right about the (partially) uninitialised variable, and 
I think there are two ways to address it:

1. By preinitialising it, i.e.:

	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
		fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
		err = user_regset_copyin(pos, count, kbuf, ubuf,
					 &fpr_val, i * sizeof(elf_fpreg_t),
					 (i + 1) * sizeof(elf_fpreg_t));
		if (err)
			return err;
		set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
	}

   but that would be an overkill given that we assert that `count' is a 
   multiple of `sizeof(elf_fpreg_t)'.

2. Actually assert what we rely on having been enforced by generic code, 
   i.e.:

	BUG_ON(*count % sizeof(elf_fpreg_t));
	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
		err = user_regset_copyin(pos, count, kbuf, ubuf,
					 &fpr_val, i * sizeof(elf_fpreg_t),
					 (i + 1) * sizeof(elf_fpreg_t));
		if (err)
			return err;
		set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
	}

   so that a discrepancy between generic code and the arch handler is 
   caught should it happen.

 Thoughts?

  Maciej

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

[Dave Martin]

On Thu, Nov 30, 2017 at 07:38:25PM +0000, Maciej W. Rozycki wrote:
> Hi Dave,
> 
> > > linux-mips-nt-prfpreg-count.diff
> > > Index: linux-sfr-test/arch/mips/kernel/ptrace.c
> > > ===================================================================
> > > --- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-21 22:12:00.000000000 +0000
> > > +++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-21 22:13:13.471970000 +0000
> > > @@ -484,7 +484,7 @@ static int fpr_set_msa(struct task_struc
> > >  	int err;
> > >  
> > >  	BUILD_BUG_ON(sizeof(fpr_val) != sizeof(elf_fpreg_t));
> > > -	for (i = 0; i < NUM_FPU_REGS && *count >= sizeof(elf_fpreg_t); i++) {
> > > +	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > > 
> > >  		err = user_regset_copyin(pos, count, kbuf, ubuf,
> > >  					 &fpr_val, i * sizeof(elf_fpreg_t),
> > >  					 (i + 1) * sizeof(elf_fpreg_t));
> > 
> > But mips*_regsets[REGSET_FPR].size == sizeof(elf_fpreg_t),
> > linux/kernel/regset.c:ptrace_regset() polices
> > iov_len % regset->size == 0, and each user_regset_copyout() call here
> > transfers sizeof(elf_fpreg_t) bytes, decrementing *count by that
> > amount unless something goest wrong in which case we return.
> 
>  Good point, I missed that check.

Understandable.  If took me a fair while to figure out how the logic
typically works here.

>  I don't think however that re-enforcing in arch code, especially in such 
> a subtle way, a constraint that has already been enforced upstream in 
> generic code is a good idea, because if we ever decide to relax the 
> constraint, then all the arch code will have to be carefully reviewed.
> 
> > If we can't end up with that, then this patch doesn't change ABI-
> > observable behaviour, unless I've missed something.
> 
>  Right, in which case there is no need to backport this change if it is 
> given a go-ahead.
> 
> > 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.

True, the commit message leaves something to be desired here.  This was
one of a bunch of patches applying similar fixes to various arches, so
I pasted the same commit message as a general description for the changes
as a set -- but you're right, it doesn't describe this spefic change
correctly here.  My bad.

>  You are of course right about the (partially) uninitialised variable, and 
> I think there are two ways to address it:
> 
> 1. By preinitialising it, i.e.:
> 
> 	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> 		fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
> 		err = user_regset_copyin(pos, count, kbuf, ubuf,
> 					 &fpr_val, i * sizeof(elf_fpreg_t),
> 					 (i + 1) * sizeof(elf_fpreg_t));
> 		if (err)
> 			return err;
> 		set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> 	}
> 
>    but that would be an overkill given that we assert that `count' is a 
>    multiple of `sizeof(elf_fpreg_t)'.

Agreed.

Was a partial write to fscr ever supported by this regset?  Your commit
message suggested that my patch may have broken that, but I can't see
how it was ever possible in the first place... unless .size has been
changed for this regset at some point.

If my patch does cause an ABI regression, then it certainly should be
fixed though.

> 2. Actually assert what we rely on having been enforced by generic code, 
>    i.e.:
> 
> 	BUG_ON(*count % sizeof(elf_fpreg_t));
> 	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> 		err = user_regset_copyin(pos, count, kbuf, ubuf,
> 					 &fpr_val, i * sizeof(elf_fpreg_t),
> 					 (i + 1) * sizeof(elf_fpreg_t));
> 		if (err)
> 			return err;
> 		set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> 	}
> 
>    so that a discrepancy between generic code and the arch handler is 
>    caught should it happen.

The important property is that *count is at least sufficient to fill
fpr_val, so that a zero return user_regset_copyin() means fpr_val has
been fully initialised with user data.

So while your check is not wrong

(since *count > 0 && *count % sizeof(elf_fpreg_t) == 0 implies
*count >= sizeof(elf_fpreg_t))

I don't see how this is an improvement on the original check.


Either way, maybe adding a comment to explain the purpose of the check
would be a good idea.

Cheers
---Dave

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

[Maciej W. Rozycki]

On Fri, 1 Dec 2017, Dave Martin wrote:

> >  You are of course right about the (partially) uninitialised variable, and 
> > I think there are two ways to address it:
> > 
> > 1. By preinitialising it, i.e.:
> > 
> > 	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > 		fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
> > 		err = user_regset_copyin(pos, count, kbuf, ubuf,
> > 					 &fpr_val, i * sizeof(elf_fpreg_t),
> > 					 (i + 1) * sizeof(elf_fpreg_t));
> > 		if (err)
> > 			return err;
> > 		set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> > 	}
> > 
> >    but that would be an overkill given that we assert that `count' is a 
> >    multiple of `sizeof(elf_fpreg_t)'.
> 
> Agreed.
> 
> Was a partial write to fscr ever supported by this regset?  Your commit
> message suggested that my patch may have broken that, but I can't see
> how it was ever possible in the first place... unless .size has been
> changed for this regset at some point.
> 
> If my patch does cause an ABI regression, then it certainly should be
> fixed though.

 I have looked into it some more, and I can see the upstream check:

	if (!regset || (kiov->iov_len % regset->size) != 0)
		return -EIO;

has always been there since the introduction of the regset feature, which 
was with commit 2225a122ae26 ("ptrace: Add support for generic 
PTRACE_GETREGSET/PTRACE_SETREGSET").  And the core file writer, i.e. 
`fill_thread_core_info', always operates on whole regsets by taking the 
size from the regset definition in question itself.

 Which means that my patch does not change the situation, but as far as 
security is concerned neither did yours, as you addressed an impossible 
case.  You did improve performance a bit though for the corner case 
situation of a partial regset access, by avoiding iterating over further 
FPRs with a call to `user_regset_copyin' once the requested transfer size 
has been exhausted.

> > 2. Actually assert what we rely on having been enforced by generic code, 
> >    i.e.:
> > 
> > 	BUG_ON(*count % sizeof(elf_fpreg_t));
> > 	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > 		err = user_regset_copyin(pos, count, kbuf, ubuf,
> > 					 &fpr_val, i * sizeof(elf_fpreg_t),
> > 					 (i + 1) * sizeof(elf_fpreg_t));
> > 		if (err)
> > 			return err;
> > 		set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> > 	}
> > 
> >    so that a discrepancy between generic code and the arch handler is 
> >    caught should it happen.
> 
> The important property is that *count is at least sufficient to fill
> fpr_val, so that a zero return user_regset_copyin() means fpr_val has
> been fully initialised with user data.
> 
> So while your check is not wrong
> 
> (since *count > 0 && *count % sizeof(elf_fpreg_t) == 0 implies
> *count >= sizeof(elf_fpreg_t))
> 
> I don't see how this is an improvement on the original check.
> 
> 
> Either way, maybe adding a comment to explain the purpose of the check
> would be a good idea.

 I agree a comment is worth having here given the complex dependency.  

 Therefore, taking what has been observed in the course of this discussion 
into account and unless either you or someone else has further input I am 
going to withdraw this patch from the series as recorded in patchwork and 
submit another one separately that only adds a comment quoting the 
observations made.  Obviously it won't be a backport candidate, but 5/5 
does not depend on this change, so I believe there is no need to 
regenerate and repost the remaining changes from this series.

 Thank you for your input.

  Maciej

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

[Maciej W. Rozycki]

On Wed, 6 Dec 2017, Maciej W. Rozycki wrote:

> > > 2. Actually assert what we rely on having been enforced by generic code, 
> > >    i.e.:
> > > 
> > > 	BUG_ON(*count % sizeof(elf_fpreg_t));
> > > 	for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > > 		err = user_regset_copyin(pos, count, kbuf, ubuf,
> > > 					 &fpr_val, i * sizeof(elf_fpreg_t),
> > > 					 (i + 1) * sizeof(elf_fpreg_t));
> > > 		if (err)
> > > 			return err;
> > > 		set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> > > 	}
> > > 
> > >    so that a discrepancy between generic code and the arch handler is 
> > >    caught should it happen.
> > 
> > The important property is that *count is at least sufficient to fill
> > fpr_val, so that a zero return user_regset_copyin() means fpr_val has
> > been fully initialised with user data.
> > 
> > So while your check is not wrong
> > 
> > (since *count > 0 && *count % sizeof(elf_fpreg_t) == 0 implies
> > *count >= sizeof(elf_fpreg_t))
> > 
> > I don't see how this is an improvement on the original check.
> > 
> > 
> > Either way, maybe adding a comment to explain the purpose of the check
> > would be a good idea.
> 
>  I agree a comment is worth having here given the complex dependency.  
> 
>  Therefore, taking what has been observed in the course of this discussion 
> into account and unless either you or someone else has further input I am 
> going to withdraw this patch from the series as recorded in patchwork and 
> submit another one separately that only adds a comment quoting the 
> observations made.  Obviously it won't be a backport candidate, but 5/5 
> does not depend on this change, so I believe there is no need to 
> regenerate and repost the remaining changes from this series.

 I take it back.  After thinking about it some more in the course of 
adding the commentary, I realised that with the addition of 2/5 we'll have 
a logical inconsistency with the treatment of `count' in that for someone 
reading this code without also looking at `ptrace_regset' it will appear 
that in the case of a partial register write we call `user_regset_copyin' 
to store FCSR having not completely exhausted `count' with FGR accesses.  

 Which means that for initial `count' being say 12 we'd put the first 8 
bytes into $f0 and the following 4 bytes into FCSR rather than into $f1, 
which I would find confusing if reading this code the first time.  This of 
course doesn't matter with our code as it is now, because it does not 
access FCSR at all, however this gets changed with 2/5.

 So I think that while such a change will have no effect at run time, 
because we guarantee that `count % sizeof(elf_fpreg_t) == 0' elsewhere, I 
think that using `count > 0' rather than `count >= sizeof(elf_fpreg_t)' 
will make code more consistent.  If you or anyone else feels uneasy about 
`fpr_val' appearing potentially partially uninitialised (even though it 
cannot trigger), then I can offer a minimal change for that, which will 
preset the variable to 0.

 Also to keep things consistent, this patch will have to be reordered 
ahead the 2/5 FCSR fix.  I will therefore submit v2 of the whole series, 
with patches shuffled appropriately, BUG_ON added as previously discussed 
as well as an explanatory comment.

  Maciej

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

[Dave Martin]

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

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

[Maciej W. Rozycki]

On Fri, 1 Dec 2017, Dave Martin wrote:

> >  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.

 That's security by obscurity, I'm afraid.

 While I do agree publicly discussing an exploitable vulnerability while 
no remedy has been yet made is considered a bad practice, I see no point 
in keeping it hidden once a fix has been developed and published.  
Moreover actually publishing all the details of the vulnerability itself 
(rather than just the fix) is often considered a good way to urge binary 
software distributors to release patched versions.

 So I think a commit description should still be as accurate as usually.

> 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.

 Well, in this situation to exploit this bug you still have to figure out 
how you can use this potential information leak in practice, so having the 
mention of a stack variable being partially initialised in the fix's 
description is IMO hardly a recipe for the potential attacker, while it 
lets the next reader understand what is really going on there.

> 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.

 Ack.

  Maciej

[PATCH 5/5] MIPS: Disallow outsized PTRACE_SETREGSET NT_PRFPREG regset accesses

[Maciej W. Rozycki]

Complement commit c23b3d1a5311 ("MIPS: ptrace: Change GP regset to use 
correct core dump register layout") and also reject outsized 
PTRACE_SETREGSET requests to the NT_PRFPREG regset, like with the 
NT_PRSTATUS regset.

Cc: stable@vger.kernel.org # v3.17+
Fixes: c23b3d1a5311 ("MIPS: ptrace: Change GP regset to use correct core dump register layout")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
 arch/mips/kernel/ptrace.c |    3 +++
 1 file changed, 3 insertions(+)

linux-mips-nt-prfpreg-size.diff
Index: linux-sfr-test/arch/mips/kernel/ptrace.c
===================================================================
--- linux-sfr-test.orig/arch/mips/kernel/ptrace.c	2017-11-28 23:52:40.373594000 +0000
+++ linux-sfr-test/arch/mips/kernel/ptrace.c	2017-11-28 23:52:47.058649000 +0000
@@ -533,6 +533,9 @@ static int fpr_set(struct task_struct *t
 	u32 fcr31;
 	int err;
 
+	if (pos + count > sizeof(elf_fpregset_t))
+		return -EIO;
+
 	init_fp_ctx(target);
 
 	if (sizeof(target->thread.fpu.fpr[0]) == sizeof(elf_fpreg_t))