From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bdMWb-0000Qe-0B for qemu-devel@nongnu.org; Fri, 26 Aug 2016 15:10:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bdMWW-0003ra-Jz for qemu-devel@nongnu.org; Fri, 26 Aug 2016 15:10:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44522) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bdMWW-0003r0-BL for qemu-devel@nongnu.org; Fri, 26 Aug 2016 15:10:40 -0400 Date: Fri, 26 Aug 2016 22:10:36 +0300 From: "Michael S. Tsirkin" Message-ID: <20160826220732-mutt-send-email-mst@kernel.org> References: <147222401281.18925.1894824578752486297.stgit@bahia.lan> <147222405454.18925.2135759955496138955.stgit@bahia.lan> <57C091D5.6050101@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57C091D5.6050101@redhat.com> 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: Greg Kurz , qemu-devel@nongnu.org, Peter Maydell , Felix Wilhelm , P J P , "Aneesh Kumar K.V" On Fri, Aug 26, 2016 at 02:00:37PM -0500, Eric Blake wrote: > On 08/26/2016 10:07 AM, Greg Kurz wrote: > > 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 crash > > 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. > > > > This patch causes the request to fail with EINVAL in these scenarios. > > > > Signed-off-by: Greg Kurz > > --- > > hw/9pfs/9p.c | 21 +++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > 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 = P9_FID_DIR; > > } else if (perm & P9_STAT_MODE_SYMLINK) { > > + if (extension.data == NULL) { > > + err = -EINVAL; > > + goto out; > > + } > > 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. > > 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 symlinks). > > http://austingroupbugs.net/view.php?id=649 Given 9p is only supported on Linux hosts, I think this patch's approach is OK for now, and it's certainly much simpler than worrying about the fallout of allowing empty names. A TODO that documents your suggestions, and including the considerations in the comment would be a good idea. > > @@ -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 = atoi(extension.data); > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > + V9fsFidState *ofidp; > > + > > + if (extension.data == NULL) { > > + err = -EINVAL; > > + goto out; > > + } > > Rejecting an empty destination on hard link or device creation, however, > is indeed appropriate. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >