[PATCH 3.18 43/52] MIPS: memset.S: EVA & fault support for small_memset

[PATCH 3.18 43/52] MIPS: memset.S: EVA & fault support for small_memset

[Greg Kroah-Hartman]

3.18-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Matt Redfearn <matt.redfearn@mips.com>

commit 8a8158c85e1e774a44fbe81106fa41138580dfd1 upstream.

The MIPS kernel memset / bzero implementation includes a small_memset
branch which is used when the region to be set is smaller than a long (4
bytes on 32bit, 8 bytes on 64bit). The current small_memset
implementation uses a simple store byte loop to write the destination.
There are 2 issues with this implementation:

1. When EVA mode is active, user and kernel address spaces may overlap.
Currently the use of the sb instruction means kernel mode addressing is
always used and an intended write to userspace may actually overwrite
some critical kernel data.

2. If the write triggers a page fault, for example by calling
__clear_user(NULL, 2), instead of gracefully handling the fault, an OOPS
is triggered.

Fix these issues by replacing the sb instruction with the EX() macro,
which will emit EVA compatible instuctions as required. Additionally
implement a fault fixup for small_memset which sets a2 to the number of
bytes that could not be cleared (as defined by __clear_user).

Reported-by: Chuanhua Lei <chuanhua.lei@intel.com>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org
Patchwork: https://patchwork.linux-mips.org/patch/18975/
Signed-off-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 arch/mips/lib/memset.S |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -178,7 +178,7 @@
 1:	PTR_ADDIU	a0, 1			/* fill bytewise */
 	R10KCBARRIER(0(ra))
 	bne		t1, a0, 1b
-	sb		a1, -1(a0)
+	 EX(sb, a1, -1(a0), .Lsmall_fixup\@)
 
 2:	jr		ra			/* done */
 	move		a2, zero
@@ -212,6 +212,11 @@
 	jr		ra
 	andi		v1, a2, STORMASK
 
+.Lsmall_fixup\@:
+	PTR_SUBU	a2, t1, a0
+	jr		ra
+	 PTR_ADDIU	a2, 1
+
 	.endm
 
 /*

[PATCH 3.18 44/52] MIPS: memset.S: Fix return of __clear_user from Lpartial_fixup

[Greg Kroah-Hartman]

3.18-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Matt Redfearn <matt.redfearn@mips.com>

commit daf70d89f80c6e1772233da9e020114b1254e7e0 upstream.

The __clear_user function is defined to return the number of bytes that
could not be cleared. From the underlying memset / bzero implementation
this means setting register a2 to that number on return. Currently if a
page fault is triggered within the memset_partial block, the value
loaded into a2 on return is meaningless.

The label .Lpartial_fixup\@ is jumped to on page fault. In order to work
out how many bytes failed to copy, the exception handler should find how
many bytes left in the partial block (andi a2, STORMASK), add that to
the partial block end address (a2), and subtract the faulting address to
get the remainder. Currently it incorrectly subtracts the partial block
start address (t1), which has additionally been clobbered to generate a
jump target in memset_partial. Fix this by adding the block end address
instead.

This issue was found with the following test code:
      int j, k;
      for (j = 0; j < 512; j++) {
        if ((k = clear_user(NULL, j)) != j) {
           pr_err("clear_user (NULL %d) returned %d\n", j, k);
        }
      }
Which now passes on Creator Ci40 (MIPS32) and Cavium Octeon II (MIPS64).

Suggested-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org
Patchwork: https://patchwork.linux-mips.org/patch/19108/
Signed-off-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -204,7 +204,7 @@
 	PTR_L		t0, TI_TASK($28)
 	andi		a2, STORMASK
 	LONG_L		t0, THREAD_BUADDR(t0)
-	LONG_ADDU	a2, t1
+	LONG_ADDU	a2, a0
 	jr		ra
 	LONG_SUBU	a2, t0
 

[PATCH 3.18 45/52] MIPS: memset.S: Fix clobber of v1 in last_fixup

[Greg Kroah-Hartman]

3.18-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Matt Redfearn <matt.redfearn@mips.com>

commit c96eebf07692e53bf4dd5987510d8b550e793598 upstream.

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.

Reported-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org
Patchwork: https://patchwork.linux-mips.org/patch/19109/
Signed-off-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -210,7 +210,7 @@
 
 .Llast_fixup\@:
 	jr		ra
-	andi		v1, a2, STORMASK
+	 nop
 
 .Lsmall_fixup\@:
 	PTR_SUBU	a2, t1, a0

Re: [PATCH 3.18 45/52] MIPS: memset.S: Fix clobber of v1 in last_fixup

[Heiher]

Hi,

IIRC, The v1 is a temporary register, value is not preserved across
function calls.

I don't see any functions that generated by compiler to restore values
of v1 after clobbered it.

On Sun, Apr 22, 2018 at 9:54 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 3.18-stable review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: Matt Redfearn <matt.redfearn@mips.com>
>
> commit c96eebf07692e53bf4dd5987510d8b550e793598 upstream.
>
> 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.
>
> Reported-by: James Hogan <jhogan@kernel.org>
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
> Cc: stable@vger.kernel.org
> Patchwork: https://patchwork.linux-mips.org/patch/19109/
> Signed-off-by: James Hogan <jhogan@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>  arch/mips/lib/memset.S |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/arch/mips/lib/memset.S
> +++ b/arch/mips/lib/memset.S
> @@ -210,7 +210,7 @@
>
>  .Llast_fixup\@:
>         jr              ra
> -       andi            v1, a2, STORMASK
> +        nop
>
>  .Lsmall_fixup\@:
>         PTR_SUBU        a2, t1, a0
>
>
>



-- 
Best regards!
Hev
https://hev.cc

Re: [PATCH 3.18 45/52] MIPS: memset.S: Fix clobber of v1 in last_fixup

[Matt Redfearn]

On 23/04/18 08:16, Heiher wrote:
> Hi,
> 
> IIRC, The v1 is a temporary register, value is not preserved across
> function calls.

v1 is conventionally used for a function return value and as such can be 
changed by called functions. However, bzero is called from inline 
assembly and v1 is not in the clobbers list
https://elixir.bootlin.com/linux/v4.17-rc1/source/arch/mips/include/asm/uaccess.h#L652
So the calling function does not expect that register to have been used 
and can legitimately expect its value to remain after the function call, 
which without this patch, it does not - as demonstrated by the test code.

Thanks,
Matt

> 
> I don't see any functions that generated by compiler to restore values
> of v1 after clobbered it.
> 
> On Sun, Apr 22, 2018 at 9:54 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> 3.18-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Matt Redfearn <matt.redfearn@mips.com>
>>
>> commit c96eebf07692e53bf4dd5987510d8b550e793598 upstream.
>>
>> 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.
>>
>> Reported-by: James Hogan <jhogan@kernel.org>
>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>> Cc: Ralf Baechle <ralf@linux-mips.org>
>> Cc: linux-mips@linux-mips.org
>> Cc: stable@vger.kernel.org
>> Patchwork: https://patchwork.linux-mips.org/patch/19109/
>> Signed-off-by: James Hogan <jhogan@kernel.org>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> ---
>>   arch/mips/lib/memset.S |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- a/arch/mips/lib/memset.S
>> +++ b/arch/mips/lib/memset.S
>> @@ -210,7 +210,7 @@
>>
>>   .Llast_fixup\@:
>>          jr              ra
>> -       andi            v1, a2, STORMASK
>> +        nop
>>
>>   .Lsmall_fixup\@:
>>          PTR_SUBU        a2, t1, a0
>>
>>
>>
> 
> 
> 

Re: [PATCH 3.18 45/52] MIPS: memset.S: Fix clobber of v1 in last_fixup

[Matt Redfearn]

On 23/04/18 08:16, Heiher wrote:
> Hi,
> 
> IIRC, The v1 is a temporary register, value is not preserved across
> function calls.

v1 is conventionally used for a function return value and as such can be 
changed by called functions. However, bzero is called from inline 
assembly and v1 is not in the clobbers list
https://elixir.bootlin.com/linux/v4.17-rc1/source/arch/mips/include/asm/uaccess.h#L652
So the calling function does not expect that register to have been used 
and can legitimately expect its value to remain after the function call, 
which without this patch, it does not - as demonstrated by the test code.

Thanks,
Matt

> 
> I don't see any functions that generated by compiler to restore values
> of v1 after clobbered it.
> 
> On Sun, Apr 22, 2018 at 9:54 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> 3.18-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Matt Redfearn <matt.redfearn@mips.com>
>>
>> commit c96eebf07692e53bf4dd5987510d8b550e793598 upstream.
>>
>> 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.
>>
>> Reported-by: James Hogan <jhogan@kernel.org>
>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>> Cc: Ralf Baechle <ralf@linux-mips.org>
>> Cc: linux-mips@linux-mips.org
>> Cc: stable@vger.kernel.org
>> Patchwork: https://patchwork.linux-mips.org/patch/19109/
>> Signed-off-by: James Hogan <jhogan@kernel.org>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> ---
>>   arch/mips/lib/memset.S |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- a/arch/mips/lib/memset.S
>> +++ b/arch/mips/lib/memset.S
>> @@ -210,7 +210,7 @@
>>
>>   .Llast_fixup\@:
>>          jr              ra
>> -       andi            v1, a2, STORMASK
>> +        nop
>>
>>   .Lsmall_fixup\@:
>>          PTR_SUBU        a2, t1, a0
>>
>>
>>
> 
> 
> 

Re: [PATCH 3.18 45/52] MIPS: memset.S: Fix clobber of v1 in last_fixup

[Heiher]

Hi,

Now I understand, thank you explain.

On Mon, Apr 23, 2018 at 5:36 PM, Matt Redfearn <matt.redfearn@mips.com> wrote:
>
>
> On 23/04/18 08:16, Heiher wrote:
>>
>> Hi,
>>
>> IIRC, The v1 is a temporary register, value is not preserved across
>> function calls.
>
>
> v1 is conventionally used for a function return value and as such can be
> changed by called functions. However, bzero is called from inline assembly
> and v1 is not in the clobbers list
> https://elixir.bootlin.com/linux/v4.17-rc1/source/arch/mips/include/asm/uaccess.h#L652
> So the calling function does not expect that register to have been used and
> can legitimately expect its value to remain after the function call, which
> without this patch, it does not - as demonstrated by the test code.
>
> Thanks,
> Matt
>
>
>>
>> I don't see any functions that generated by compiler to restore values
>> of v1 after clobbered it.
>>
>> On Sun, Apr 22, 2018 at 9:54 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> 3.18-stable review patch.  If anyone has any objections, please let me
>>> know.
>>>
>>> ------------------
>>>
>>> From: Matt Redfearn <matt.redfearn@mips.com>
>>>
>>> commit c96eebf07692e53bf4dd5987510d8b550e793598 upstream.
>>>
>>> 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.
>>>
>>> Reported-by: James Hogan <jhogan@kernel.org>
>>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>>> Cc: Ralf Baechle <ralf@linux-mips.org>
>>> Cc: linux-mips@linux-mips.org
>>> Cc: stable@vger.kernel.org
>>> Patchwork: https://patchwork.linux-mips.org/patch/19109/
>>> Signed-off-by: James Hogan <jhogan@kernel.org>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>
>>> ---
>>>   arch/mips/lib/memset.S |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> --- a/arch/mips/lib/memset.S
>>> +++ b/arch/mips/lib/memset.S
>>> @@ -210,7 +210,7 @@
>>>
>>>   .Llast_fixup\@:
>>>          jr              ra
>>> -       andi            v1, a2, STORMASK
>>> +        nop
>>>
>>>   .Lsmall_fixup\@:
>>>          PTR_SUBU        a2, t1, a0
>>>
>>>
>>>
>>
>>
>>
>



-- 
Best regards!
Hev
https://hev.cc