Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Eric Blake]

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

> @@ -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

signature.asc
Description: OpenPGP digital signature