[PATCH v2 1/4] MIPS: memset.S: Fix clobber of v1 in last_fixup

[PATCH v2 1/4] MIPS: memset.S: Fix clobber of v1 in last_fixup

[Matt Redfearn]

The label .Llast_fixup\@ is jumped to on page fault within the final
byte set loop of memset (on < MIPSR6 architectures). For some reason, in
this fault handler, the v1 register is randomly set to a2 & STORMASK.
This clobbers v1 for the calling function. This can be observed with the
following test code:

static int __init __attribute__((optimize("O0"))) test_clear_user(void)
{
  register int t asm("v1");
  char *test;
  int j, k;

  pr_info("\n\n\nTesting clear_user\n");
  test = vmalloc(PAGE_SIZE);

  for (j = 256; j < 512; j++) {
    t = 0xa5a5a5a5;
    if ((k = clear_user(test + PAGE_SIZE - 256, j)) != j - 256) {
        pr_err("clear_user (%px %d) returned %d\n", test + PAGE_SIZE - 256, j, k);
    }
    if (t != 0xa5a5a5a5) {
       pr_err("v1 was clobbered to 0x%x!\n", t);
    }
  }

  return 0;
}
late_initcall(test_clear_user);

Which demonstrates that v1 is indeed clobbered (MIPS64):

Testing clear_user
v1 was clobbered to 0x1!
v1 was clobbered to 0x2!
v1 was clobbered to 0x3!
v1 was clobbered to 0x4!
v1 was clobbered to 0x5!
v1 was clobbered to 0x6!
v1 was clobbered to 0x7!

Since the number of bytes that could not be set is already contained in
a2, the andi placing a value in v1 is not necessary and actively
harmful in clobbering v1.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
---

Changes in v2: None

 arch/mips/lib/memset.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/lib/memset.S b/arch/mips/lib/memset.S
index 184819c1d5c8..f7327979a8f8 100644
--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -258,7 +258,7 @@
 
 .Llast_fixup\@:
 	jr		ra
-	andi		v1, a2, STORMASK
+	 nop
 
 .Lsmall_fixup\@:
 	PTR_SUBU	a2, t1, a0
-- 
2.7.4

[PATCH v2 2/4] MIPS: uaccess: Add micromips clobbers to bzero invocation

[Matt Redfearn]

The micromips implementation of bzero additionally clobbers registers t7
& t8. Specify this in the clobbers list when invoking bzero.

Reported-by: James Hogan <jhogan@kernel.org>
Fixes: 26c5e07d1478 ("MIPS: microMIPS: Optimise 'memset' core library function.")
Cc: stable@vger.kernel.org
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
---

Changes in v2: None

 arch/mips/include/asm/uaccess.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
index b71306947290..06629011a434 100644
--- a/arch/mips/include/asm/uaccess.h
+++ b/arch/mips/include/asm/uaccess.h
@@ -654,6 +654,13 @@ __clear_user(void __user *addr, __kernel_size_t size)
 {
 	__kernel_size_t res;
 
+#ifdef CONFIG_CPU_MICROMIPS
+/* micromips memset / bzero also clobbers t7 & t8 */
+#define bzero_clobbers "$4", "$5", "$6", __UA_t0, __UA_t1, "$15", "$24", "$31"
+#else
+#define bzero_clobbers "$4", "$5", "$6", __UA_t0, __UA_t1, "$31"
+#endif /* CONFIG_CPU_MICROMIPS */
+
 	if (eva_kernel_access()) {
 		__asm__ __volatile__(
 			"move\t$4, %1\n\t"
@@ -663,7 +670,7 @@ __clear_user(void __user *addr, __kernel_size_t size)
 			"move\t%0, $6"
 			: "=r" (res)
 			: "r" (addr), "r" (size)
-			: "$4", "$5", "$6", __UA_t0, __UA_t1, "$31");
+			: bzero_clobbers);
 	} else {
 		might_fault();
 		__asm__ __volatile__(
@@ -674,7 +681,7 @@ __clear_user(void __user *addr, __kernel_size_t size)
 			"move\t%0, $6"
 			: "=r" (res)
 			: "r" (addr), "r" (size)
-			: "$4", "$5", "$6", __UA_t0, __UA_t1, "$31");
+			: bzero_clobbers);
 	}
 
 	return res;
-- 
2.7.4

Re: [PATCH v2 1/4] MIPS: memset.S: Fix clobber of v1 in last_fixup

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

On Tue, Apr 17, 2018 at 04:40:00PM +0100, Matt Redfearn wrote:
> The label .Llast_fixup\@ is jumped to on page fault within the final
> byte set loop of memset (on < MIPSR6 architectures). For some reason, in
> this fault handler, the v1 register is randomly set to a2 & STORMASK.
> This clobbers v1 for the calling function. This can be observed with the
> following test code:
> 
> static int __init __attribute__((optimize("O0"))) test_clear_user(void)
> {
>   register int t asm("v1");
>   char *test;
>   int j, k;
> 
>   pr_info("\n\n\nTesting clear_user\n");
>   test = vmalloc(PAGE_SIZE);
> 
>   for (j = 256; j < 512; j++) {
>     t = 0xa5a5a5a5;
>     if ((k = clear_user(test + PAGE_SIZE - 256, j)) != j - 256) {
>         pr_err("clear_user (%px %d) returned %d\n", test + PAGE_SIZE - 256, j, k);
>     }
>     if (t != 0xa5a5a5a5) {
>        pr_err("v1 was clobbered to 0x%x!\n", t);
>     }
>   }
> 
>   return 0;
> }
> late_initcall(test_clear_user);
> 
> Which demonstrates that v1 is indeed clobbered (MIPS64):
> 
> Testing clear_user
> v1 was clobbered to 0x1!
> v1 was clobbered to 0x2!
> v1 was clobbered to 0x3!
> v1 was clobbered to 0x4!
> v1 was clobbered to 0x5!
> v1 was clobbered to 0x6!
> v1 was clobbered to 0x7!
> 
> Since the number of bytes that could not be set is already contained in
> a2, the andi placing a value in v1 is not necessary and actively
> harmful in clobbering v1.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Reported-by: James Hogan <jhogan@kernel.org>
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>

Thanks, Patches 1 & 2 applied to my fixes branch.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]