From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boqun Feng Subject: Re: [PATCH v2] vfs: avoid recopying file names in getname_flags Date: Mon, 13 Apr 2015 15:34:45 +0800 Message-ID: <20150413073445.GC25517@fixme-laptop.cn.ibm.com> References: <1427309152-25129-1-git-send-email-boqun.feng@gmail.com> <20150329042744.GA1477@fixme-laptop> <20150407083826.GA1048@fixme-laptop.cn.ibm.com> <20150411235655.GK889@ZenIV.linux.org.uk> <20150412011318.GL889@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ABTtc+pdwF7KHXCz" Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Paul Moore To: Al Viro Return-path: Content-Disposition: inline In-Reply-To: <20150412011318.GL889@ZenIV.linux.org.uk> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org --ABTtc+pdwF7KHXCz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Apr 12, 2015 at 02:13:18AM +0100, Al Viro wrote: >=20 > BTW, looking at the __getname() callers... Lustre one sure as hell looks > bogus: > char *tmp =3D __getname(); >=20 > if (!tmp) > return ERR_PTR(-ENOMEM); >=20 > len =3D strncpy_from_user(tmp, filename, PATH_MAX); > if (len =3D=3D 0) > ret =3D -ENOENT; > else if (len > PATH_MAX) > ret =3D -ENAMETOOLONG; >=20 > if (ret) { > __putname(tmp); > tmp =3D ERR_PTR(ret); > } > return tmp; >=20 > Note that > * strncpy_from_user(p, u, n) can return a negative (-EFAULT) > * strncpy_from_user(p, u, n) cannot return a positive greater than > n. In case of missing NUL among the n bytes starting at u (and no faults > accessing those) we get exactly n bytes copied and n returned. In case > when NUL is there, we copy everything up to and including that NUL and > return number of non-NUL bytes copied. >=20 > IOW, these failure cases had never been tested. Name being too long ends= up > with non-NUL-terminated array of characters returned, and the very first > caller of ll_getname() feeds it to strlen(). Fault ends up with uninitia= lized > array... Yeah.. and it's suprising to see it doesn't make any trouble yet. >=20 > AFAICS, the damn thing should just use getname() and quit reinventing the > wheel, badly. I cscoped the kernel code and find 15 __getname() callers, they use the memory that __getname()s return in quite different ways. But at least we can divide them into two groups, 1) fill the memory with names from user space 2) fill the memory with names from kernel space. For 1) we can use getname() to do the job and for 2) I think first we need to figure how they are using the memory, because they may generate names in different ways, and clean them one by one if they need to be. >=20 > As for your question in another thread - AFAICS, it's impossible with the > current code, but not too robust. Fortunately, it's trivial to make > independent on allocator details - all it takes is > result =3D kzalloc(offsetof(struct filename, iname[1]), GFP_KERNEL); > and we are done - result->iname+1 will be within the allocated object, > no matter what. Thank you for your response. As long as the actual size of result is not a power of 2, the problem will not happen.(Maybe add a comment before struct filename) Regards, Boqun Feng --ABTtc+pdwF7KHXCz Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJVK3GRAAoJEEl56MO1B/q4LpoH/RLxUqmDjxUytSf7Pd3T5fHY R0OQSgvnC1Xr460NKF9ITtNe2BRaD3i9IjZ0DJZsVtc5qKdKSF115aSUsvqTV5BZ mw7WSQ8DP+/9pZmpzQxqmQbQqlhUq65BDf5CErVAIwpt1Ky6qSOfZOaSY/Voudh+ fGXc6rTvM9E0SHh6SqqDAKBbSkSQVvp2MfbVSfPixEw/2+H45vlO2zr87srnSatx TsdCUJ+01Hp2cCvgUgNqa8y1arM/vXpMf8hx9ddyflhHhMrlTtY3mYj6X2b6eWIW 5PG4UW5Z0D6BMG4vYl0mOVeiHQOQYD+fzaa1bDP3sbIfRMp9IX/4C1UwxfzJl9Y= =XeSy -----END PGP SIGNATURE----- --ABTtc+pdwF7KHXCz--