From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Hogan Subject: Re: [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user Date: Wed, 5 Apr 2017 07:54:14 +0100 Message-ID: <20170405065414.GL31606@jhogan-linux.le.imgtec.org> References: <20170404163157.GO29622@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7BtE0xW96okrVgVt" Return-path: Content-Disposition: inline In-Reply-To: <20170404163157.GO29622-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> Sender: linux-metag-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: To: Al Viro Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org --7BtE0xW96okrVgVt Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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", \ >=20 > 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 t= hat were > > + * inaccessible. > > + */ > > +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc, > > + unsigned long n) > > { > > register char *dst asm ("A0.2") =3D pdst; > > register const char __user *src asm ("A1.2") =3D 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; > > } > > } >=20 > 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 + re= tn > returned, reporting that you've copied a single byte. Which you certainly > have, but it's pdst[1] =3D psrc[1], not pdst[0] =3D psrc[0]. >=20 > 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 --7BtE0xW96okrVgVt Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJY5JR6AAoJEGwLaZPeOHZ6HZQQAKuakmWt02c2i5d9jrv37avW DB/wMnwQYRkumgjdw0zyjFQgAfQXFL1oLWUq3Opx+pVgp1xmoD0JCfahUg1t8Jt3 7zfGCN4dDd+5lw7DeyA9EkyCwV3ZNWt5LxaWZwDsIezxp7H9gkFyc+DgCyHIaSUm RUhzUoDhncX7Sk5j3zcDGZrg4pil3umLJYkAOciqy1JNDNbWu4uX8yzdm67pgpsZ sVFiTB5cndXQ5GbqThUiXwL2sWmxkOJy9G3nyL5ZyCjYxJ4G3Pg9g0dakZIcjgtL YEVFVgu7fKVaw42/CYHHHDD6AdUSdoy80i7bVv3J+drDDgx3GNVWnReFTNvXoUiU ZNpnbY9AGSlKUOIgFV61krRJ239jqTiUI7zXKaO09n5R1NoamqY+U/L3RTQxhOYE M8326xSDEjywIWh/NJFrdnlNeBtgjH7QhMQB5p20L8uoLaJTV8Qswlqy0JEERNcA P2cNddrxURMF21ZBvLILSZ0ar8HgDXoKKtREqpIR5E+WFpFBV/U/LYDj+8+NYDjK DTFxXgNoIrkF1XV97dUjK7x4VcXeyqEJeX1x1p+2RGeoisaUpbb7udwfOvmv8s7A 0PUo1uvh/F+B62Ufas8zIQNJXGXVqrWf5NKp8pGnreLORI30ivuktSwsPKXIPiGS 2PDRbVloI4cuCuUMB3kI =ruot -----END PGP SIGNATURE----- --7BtE0xW96okrVgVt--