[PATCH] microblaze: fix /dev/zero corruption from __clear_user()

[PATCH] microblaze: fix /dev/zero corruption from __clear_user()

[Steven J. Magnani]

A userland read of more than PAGE_SIZE bytes from /dev/zero results in
(a) not all of the bytes returned being zero, and
(b) memory corruption due to zeroing of bytes beyond the user buffer.

This is caused by improper constraints on the assembly __clear_user function.
The constrints don't indicate to the compiler that the pointer argument is
modified. Since the function is inline, this results in double-incrementing
of the pointer when __clear_user() is invoked through a multi-page read() of
/dev/zero. 

Signed-off-by: Steven J. Magnani <steve@digidescorp.com>
---
diff -uprN a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
--- a/arch/microblaze/include/asm/uaccess.h	2011-02-10 11:51:52.319226513 -0600
+++ b/arch/microblaze/include/asm/uaccess.h	2011-02-10 11:53:01.064857179 -0600
@@ -120,16 +120,16 @@ static inline unsigned long __must_check
 {
 	/* normal memset with two words to __ex_table */
 	__asm__ __volatile__ (				\
-			"1:	sb	r0, %2, r0;"	\
+			"1:	sb	r0, %1, r0;"	\
 			"	addik	%0, %0, -1;"	\
 			"	bneid	%0, 1b;"	\
-			"	addik	%2, %2, 1;"	\
+			"	addik	%1, %1, 1;"	\
 			"2:			"	\
 			__EX_TABLE_SECTION		\
 			".word	1b,2b;"			\
 			".previous;"			\
-		: "=r"(n)				\
-		: "0"(n), "r"(to)
+		: "=r"(n), "=r"(to)			\
+		: "0"(n), "1"(to)
 	);
 	return n;
 }

Re: [PATCH] microblaze: fix /dev/zero corruption from __clear_user()

[Michal Simek]

Steven J. Magnani wrote:
> A userland read of more than PAGE_SIZE bytes from /dev/zero results in
> (a) not all of the bytes returned being zero, and
> (b) memory corruption due to zeroing of bytes beyond the user buffer.
> 
> This is caused by improper constraints on the assembly __clear_user function.
> The constrints don't indicate to the compiler that the pointer argument is
> modified. Since the function is inline, this results in double-incrementing
> of the pointer when __clear_user() is invoked through a multi-page read() of
> /dev/zero. 
> 
> Signed-off-by: Steven J. Magnani <steve@digidescorp.com>

I wasn't able to reproduce this issue on MMU and noMMU systems (log below).
I think it is toolchain issue.

Anyway it will be the best not to increment "to" at all and use "n" as offset.
This could also speed it up this function.
There is also possibility to add sb to delay slot but it works from Microblaze 5.00.a and later.

I will add this patch to my next branch and will be good to get some statistic and then to write
some optimization for it.

Acked-by: Michal Simek <monstr@monstr.eu>

Thanks,
Michal


My log:
# dd if=/dev/zero of=/tmp/1 bs=64k count=1
1+0 records in
1+0 records out
~ # hexdump /tmp/1
0000000 0000 0000 0000 0000 0000 0000 0000 0000
*
0010000
~ # md5sum /tmp/1
fcd6bcb56c1689fcef28b57c22475bad  /tmp/1
~ # cat /proc/cpuinfo
CPU-Family:     MicroBlaze
FPGA-Arch:      spartan6
CPU-Ver:        7.30.a, big endian
CPU-MHz:        90.00
BogoMips:       44.54
HW:
  Shift:         yes
  MSR:           yes
  PCMP:          yes
  DIV:           no
  MMU:           3
  MUL:           v2
  FPU:           no
  Exc:           ill
Icache:         16kB    line length:    16B
Dcache:         16kB    line length:    16B
                 write-through
HW-Debug:       yes
PVR-USR1:       00
PVR-USR2:       00000000
Page size:      16384
~ #

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian