Re: [PATCH 18/31] nds32: Library functions

On Wed, Nov 08, 2017 at 01:55:06PM +0800, Greentime Hu wrote:

+#define __range_ok(addr, size) (size <= get_fs() && addr <= (get_fs() -size))
+
+#define access_ok(type, addr, size)                 \
+	__range_ok((unsigned long)addr, (unsigned long)size)

+#define __get_user_x(__r2,__p,__e,__s,__i...)				\
+	   __asm__ __volatile__ (					\
+		__asmeq("%0", "$r0") __asmeq("%1", "$r2")		\
+		"bal	__get_user_" #__s				\

... which does not check access_ok() or do any visible equivalents; OK...

+#define get_user(x,p)							\
+	({								\
+		const register typeof(*(p)) __user *__p asm("$r0") = (p);\
+		register unsigned long __r2 asm("$r2");			\
+		register int __e asm("$r0");				\
+		switch (sizeof(*(__p))) {				\
+		case 1:							\
+			__get_user_x(__r2, __p, __e, 1, "$lp");		\

... and neither does this, which is almost certainly *not* OK.

+#define put_user(x,p)							\

Same here, AFAICS.

+extern unsigned long __arch_copy_from_user(void *to, const void __user * from,
+					   unsigned long n);

+static inline unsigned long raw_copy_from_user(void *to,
+					       const void __user * from,
+					       unsigned long n)
+{
+	return __arch_copy_from_user(to, from, n);
+}

Er...  Why not call your __arch_... raw_... and be done with that?

+#define INLINE_COPY_FROM_USER
+#define INLINE_COPY_TO_USER

Are those actually worth bothering?  IOW, have you compared behaviour
with and without them?

+ENTRY(__arch_copy_to_user)
+	push	$r0
+	push	$r2
+	beqz	$r2, ctu_exit
+	srli	$p0, $r2, #2		! $p0 = number of word to clear
+	andi	$r2, $r2, #3		! Bytes less than a word to copy
+	beqz	$p0, byte_ctu		! Only less than a word to copy
+word_ctu:
+	lmw.bim	$p1, [$r1], $p1		! Load the next word
+USER(	smw.bim,$p1, [$r0], $p1)	! Store the next word

Umm...  It's that happy with unaligned loads and stores?  Your memcpy seems
to be trying to avoid those...

+9001:
+	pop	$p1			! Original $r2, n
+	pop	$p0			! Original $r0, void *to
+	sub	$r1, $r0, $p0		! Bytes copied
+	sub	$r2, $p1, $r1		! Bytes left to copy
+	push	$lp
+	move	$r0, $p0
+	bal	memzero			! Clean up the memory

Just what memory are you zeroing here?  The one you had been
unable to store into in the first place?

+ENTRY(__arch_copy_from_user)

+9001:
+	pop	$p1			! Original $r2, n
+	pop	$p0			! Original $r0, void *to
+	sub	$r1, $r1, $p0		! Bytes copied
+	sub	$r2, $p1, $r1		! Bytes left to copy
+	push	$lp
+	bal	memzero			! Clean up the memory

Ditto, only this one is even worse - instead of just oopsing on
you, it will quietly destroy data past the area you've copied
into.  raw_copy_..._user() MUST NOT ZERO ANYTHING.  Ever.