[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string |
Date: |
Fri, 26 Aug 2016 22:10:36 +0300 |
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 <address@hidden>
> > ---
> > 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
>
- Re: [Qemu-devel] [PATCH v2 2/5] 9p: disallow the NUL character in all strings, (continued)