On Tue, Apr 04, 2017 at 05:31:57PM +0100, Al Viro wrote:
> 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?
Mainly for consistency with the current expectations of these macros
(otherwise we'd break the unaligned handling of valid copies), but I
think I agree that if the issue mentioned below if fixed before this
patch we could remove these adds, since it'll always result in an
immediate return based on n and retn (and not dst).
> > +/*
> > + * 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.
Agreed. This is one of the other fixes I mentioned. I'll rearrange to
put that fix first.
> 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...
Thanks for reviewing
Cheers
James
Attachment:
signature.asc
Description: Digital signature