From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39470) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1be3z9-0005Lb-8c for qemu-devel@nongnu.org; Sun, 28 Aug 2016 13:35:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1be3z4-0002Gu-6q for qemu-devel@nongnu.org; Sun, 28 Aug 2016 13:35:07 -0400 Received: from 3.mo6.mail-out.ovh.net ([178.33.253.26]:35582) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1be3z3-0002GO-UL for qemu-devel@nongnu.org; Sun, 28 Aug 2016 13:35:02 -0400 Received: from player786.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo6.mail-out.ovh.net (Postfix) with ESMTP id B1C1AFF8BB6 for ; Sun, 28 Aug 2016 19:35:00 +0200 (CEST) Date: Sun, 28 Aug 2016 19:34:49 +0200 From: Greg Kurz Message-ID: <20160828193449.0bcde2f7@bahia.lan> In-Reply-To: <20160828192125.1f1ad6f9@bahia.lan> References: <147222401281.18925.1894824578752486297.stgit@bahia.lan> <147222405454.18925.2135759955496138955.stgit@bahia.lan> <57C091D5.6050101@redhat.com> <20160828192125.1f1ad6f9@bahia.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/r8VbBFgJMoJouOMgzaVA=7a"; protocol="application/pgp-signature" Subject: Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Peter Maydell , Felix Wilhelm , "Michael S. Tsirkin" , qemu-devel@nongnu.org, P J P , "Aneesh Kumar K.V" --Sig_/r8VbBFgJMoJouOMgzaVA=7a Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Sun, 28 Aug 2016 19:21:25 +0200 Greg Kurz wrote: > On Fri, 26 Aug 2016 14:00:37 -0500 > Eric Blake wrote: >=20 > > On 08/26/2016 10:07 AM, Greg Kurz wrote: =20 > > > A buggy guest using the 9p2000.u protocol can issue a create request = and > > > pass an empty string as the extension argument. This causes QEMU to c= rash > > > in the case of a hard link or a special file, and leads to undefined > > > behavior, depending on the backend, in the case of a symbolic link. > > >=20 > > > This patch causes the request to fail with EINVAL in these scenarios. > > >=20 > > > Signed-off-by: Greg Kurz > > > --- =20 >=20 > Wait... empty strings coming from pdu_unmarshal() never have data =3D=3D = NULL > so this whole patch is pointless :) and BTW, only the symlink case is abo= ut > file names. >=20 > > > hw/9pfs/9p.c | 21 +++++++++++++++++++-- > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > >=20 > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > index 7b1dfe4e47cb..dc65c3125006 100644 > > > --- a/hw/9pfs/9p.c > > > +++ b/hw/9pfs/9p.c > > > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque) > > > } > > > fidp->fid_type =3D P9_FID_DIR; > > > } else if (perm & P9_STAT_MODE_SYMLINK) { > > > + if (extension.data =3D=3D NULL) { > > > + err =3D -EINVAL; > > > + goto out; > > > + } =20 > > =20 >=20 > I realize that this part belongs to patch 1 actually: it is the implement= ation of > symbolic links that comes with 9P2000.u (different from the TSYMLINK requ= est in > 9P2000.L). In which case, the hunk would have been: >=20 > + if (name_is_illegal(extension.data)) { > + err =3D -EINVAL; > + goto out; > + } >=20 > > POSIX specifically requires implementations to support creating a > > symlink whose target is the empty string. Linux doesn't [yet] permit > > it, but BSD does. On systems where creating such a symlink is legal, > > POSIX requires that such a symlink either be treated as "." if > > dereferenced, or be treated as ENOENT on attempt to dereference. But > > since such links can be created, readlink() should be able to read them > > without error. > >=20 > > I would argue that we should NOT forbid empty symlinks on creation (but > > pass back any error from the underlying host OS); but instead check that > > dereferencing such a symlink behaves sanely if it was created. > > Meanwhile, a client should not be relying on the behavior (since Linux > > disobeys POSIX, portable clients should already be avoiding empty symli= nks). > >=20 > > http://austingroupbugs.net/view.php?id=3D649 > > =20 >=20 > Indeed, maybe we should let the backend decide if it allows symlink with > an empty target, since the target name is simply "stored" somewhere and > not used to create a new path. In which case, we should do the same with > v9fs_symlink(). And we would have two exceptions to the name_is_illegal() > helper, because we still want to avoid '/' in file names... >=20 Thinking again... I guess '/' in names should result in ENOENT since it see= ms to be a common way to say something is wrong with a path... or we should ha= ve separate error paths between the '/'-in-names and the empty name cases. > On the other hand, we only support linux hosts where the call to symlink() > will fail with ENOENT and guests using the official linux kernel 9p client > never send an empty target... >=20 > For the sake of simplicity, I'd rather have the target names to follow the > same rules as other file names, and return ENOENT directly (the link you > provide states it is a valid option). >=20 > Peter, >=20 > Since you suggested to do explicit error checking on empty file names, do > you have an opinion on the case of symlinks with an empty target ? >=20 > > > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque) > > > } > > > v9fs_path_copy(&fidp->path, &path); > > > } else if (perm & P9_STAT_MODE_LINK) { > > > - int32_t ofid =3D atoi(extension.data); > > > - V9fsFidState *ofidp =3D get_fid(pdu, ofid); > > > + V9fsFidState *ofidp; > > > + > > > + if (extension.data =3D=3D NULL) { > > > + err =3D -EINVAL; > > > + goto out; > > > + } =20 > >=20 > > Rejecting an empty destination on hard link or device creation, however, > > is indeed appropriate. > > =20 >=20 > In the case of hard links, extension.data is a FID, not a file name. >=20 > In the case of device creation, extension.data is "type major minor", not > a file name again. --Sig_/r8VbBFgJMoJouOMgzaVA=7a Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAlfDILkACgkQAvw66wEB28I7EgCcD3+6goKAqRx6lOEUMddV9tfU 6v8An2hhSAyFrKewWzQRNTA3wrUM4k2/ =IDR5 -----END PGP SIGNATURE----- --Sig_/r8VbBFgJMoJouOMgzaVA=7a--