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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]