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...
--
To unsubscribe from this list: send the line "unsubscribe linux-metag" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
