[1/2] MIPS: memset.S: EVA & fault support for small_memset
diff mbox series

Message ID -
State Accepted
Delegated to: James Hogan
Headers show
Series
  • MIPS: memset.S: Fix 2 issues with __clear_user
Related show

Commit Message

Matt Redfearn March 29, 2018, 9:28 a.m. UTC
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 <>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: 
Signed-off-by: Matt Redfearn <>

---

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

Comments

James Hogan April 16, 2018, 8:22 p.m. UTC | #1
On Thu, Mar 29, 2018 at 10:28:23AM +0100, Matt Redfearn wrote:
> @@ -260,6 +260,11 @@
>  	jr		ra
>  	andi		v1, a2, STORMASK

This patch looks good, well spotted!

But whats that v1 write about? Any ideas? Seems to go back to the git
epoch, and $3 isn't in the clobber lists when __bzero* is called.

Cheers
James

>  
> +.Lsmall_fixup\@:
> +	PTR_SUBU	a2, t1, a0
> +	jr		ra
> +	 PTR_ADDIU	a2, 1
> +
Matt Redfearn April 17, 2018, 1:20 p.m. UTC | #2
Hi James,

On 16/04/18 21:22, James Hogan wrote:
> On Thu, Mar 29, 2018 at 10:28:23AM +0100, Matt Redfearn wrote:
>> @@ -260,6 +260,11 @@
>>   	jr		ra
>>   	andi		v1, a2, STORMASK
> 
> This patch looks good, well spotted!
> 
> But whats that v1 write about? Any ideas? Seems to go back to the git
> epoch, and $3 isn't in the clobber lists when __bzero* is called.

No idea what the original intent was, but I've verified that v1 does 
indeed get clobbered if this path is hit. Patch incoming!

Thanks,
Matt

> 
> Cheers
> James
> 
>>   
>> +.Lsmall_fixup\@:
>> +	PTR_SUBU	a2, t1, a0
>> +	jr		ra
>> +	 PTR_ADDIU	a2, 1
>> +
Maciej W. Rozycki May 14, 2018, 10:56 p.m. UTC | #3
On Mon, 16 Apr 2018, James Hogan wrote:

> > @@ -260,6 +260,11 @@
> >  	jr		ra
> >  	andi		v1, a2, STORMASK
> 
> This patch looks good, well spotted!
> 
> But whats that v1 write about? Any ideas? Seems to go back to the git
> epoch, and $3 isn't in the clobber lists when __bzero* is called.

 You need to dive deeper, that is beyond the secret commit 66f0a432564b 
("Add resource managment."), to find what's happened before the epoch. ;)

 Anyway, there isn't anything special here, the thing has been here since 
the inception of memset.S with commit 2e0f55e79c49 (no shortlog available 
for that one).  And it is clearly a bug, possibly just a leftover from a 
WIP implementation or whatever.

 And I can see Matt has already fixed that, thanks!

  Maciej

Patch
diff mbox series

diff --git a/arch/mips/lib/memset.S b/arch/mips/lib/memset.S
index a1456664d6c2..90bcdf1224ee 100644
--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -219,7 +219,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
@@ -260,6 +260,11 @@ 
 	jr		ra
 	andi		v1, a2, STORMASK
 
+.Lsmall_fixup\@:
+	PTR_SUBU	a2, t1, a0
+	jr		ra
+	 PTR_ADDIU	a2, 1
+
 	.endm
 
 /*