Re: [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Apr 04, 2017 at 03:46:51PM +0100, James Hogan wrote:
> @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user);
>  		"	GETB D1Ar1,[%1++]\n"	\
>  		"2:	SETB [%0++],D1Ar1\n",	\
>  		"3:	ADD  %2,%2,#1\n"	\
> -		"	SETB [%0++],D1Ar1\n",	\
> +		"	ADD  %0,%0,#1\n",	\

Why bother incrementing dst here?

>  		"	.long 2b,3b\n")
>  
>  #define __asm_copy_from_user_2x_cont(to, from, ret, COPY, FIXUP, TENTRY) \
> @@ -626,7 +625,7 @@ EXPORT_SYMBOL(__copy_user);
>  		"	GETW D1Ar1,[%1++]\n"		\
>  		"2:	SETW [%0++],D1Ar1\n" COPY,	\
>  		"3:	ADD  %2,%2,#2\n"		\
> -		"	SETW [%0++],D1Ar1\n" FIXUP,	\
> +		"	ADD  %0,%0,#2\n" FIXUP,		\
>  		"	.long 2b,3b\n" TENTRY)
>  
>  #define __asm_copy_from_user_2(to, from, ret) \
> @@ -637,7 +636,7 @@ EXPORT_SYMBOL(__copy_user);
>  		"	GETB D1Ar1,[%1++]\n"		\
>  		"4:	SETB [%0++],D1Ar1\n",		\
>  		"5:	ADD  %2,%2,#1\n"		\
> -		"	SETB [%0++],D1Ar1\n",		\
> +		"	ADD  %0,%0,#1\n",		\
>  		"	.long 4b,5b\n")
>  
>  #define __asm_copy_from_user_4x_cont(to, from, ret, COPY, FIXUP, TENTRY) \
> @@ -645,23 +644,20 @@ EXPORT_SYMBOL(__copy_user);
>  		"	GETD D1Ar1,[%1++]\n"		\
>  		"2:	SETD [%0++],D1Ar1\n" COPY,	\
>  		"3:	ADD  %2,%2,#4\n"		\
> -		"	SETD [%0++],D1Ar1\n" FIXUP,	\
> +		"	ADD  %0,%0,#4\n" FIXUP,		\
>  		"	.long 2b,3b\n" TENTRY)
>
>  #define __asm_copy_from_user_8x64(to, from, ret) \
>  	asm volatile (				\
>  		"	GETL D0Ar2,D1Ar1,[%1++]\n"	\
>  		"2:	SETL [%0++],D0Ar2,D1Ar1\n"	\
>  		"1:\n"					\
>  		"	.section .fixup,\"ax\"\n"	\
> -		"	MOV D1Ar1,#0\n"			\
> -		"	MOV D0Ar2,#0\n"			\
>  		"3:	ADD  %2,%2,#8\n"		\
> -		"	SETL [%0++],D0Ar2,D1Ar1\n"	\
> +		"	ADD  %0,%0,#8\n"		\
>  		"	MOVT    D0Ar2,#HI(1b)\n"	\
>  		"	JUMP    D0Ar2,#LO(1b)\n"	\
>  		"	.previous\n"			\


Ditto for all of those, actually.

> +/*
> + * Copy from user to kernel. The return-value is the number of bytes that were
> + * inaccessible.
> + */
> +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc,
> +				 unsigned long n)
>  {
>  	register char *dst asm ("A0.2") = pdst;
>  	register const char __user *src asm ("A1.2") = psrc;
> @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc,
>  			__asm_copy_from_user_1(dst, src, retn);
>  			n--;
>  			if (retn)
> -				goto copy_exception_bytes;
> +				return retn + n;
>  		}
>  	}

Umm...  What happens if psrc points to the last byte of an unmapped page,
with psrc + 1 pointing to valid data?  AFAICS, you'll get GETB fail, have
retn increased to 1, n decremented by 1, then, assuming you end up in that
byte-copy loop, have GETB at src + 1 *succeed*, the value fetched fed to SETB
at dst + 1, n decremented again, non-zero retn finally spotted and n + retn
returned, reporting that you've copied a single byte.  Which you certainly
have, but it's pdst[1] = psrc[1], not pdst[0] = psrc[0].

IOW, you need to check retn after all __asm_copy_from_user_{1,2} in the
beginning.

As for topic branch for #1 and #2 - sure, perfectly fine by me.  Just give
me the branch name and I'll pull.  But I think the scenario above needs to
be dealt with...