[Qemu-devel] [PATCH RFC 00/36] 9pfs: local: fix vulnerability to symlink attacks

[Qemu-devel] [PATCH RFC 00/36] 9pfs: local: fix vulnerability to symlink attacks

[Greg Kurz]

This series tries to fix CVE-2016-9602. This vulnerability affects all
accesses to the underlying filesystem in the "local" backend code.

If QEMU is started with:

-fsdev local,security_model=<passthrough|none>,path=/foo/bar

then the guest can cause QEMU to create symlinks in /foo/bar.

This causes accesses to any path /foo/bar/some/path to be unsafe, since
untrusted code within the guest (or in another guest sharing the same
virtfs folder) could change some/path to point to a random path of the
host filesystem.

The core problem is that the "local" backend relies on path-based syscalls
to access the underlying filesystem. All path-based syscalls are vulnerable
to this issue, even open(O_NOFOLLOW) or syscalls that explicitly don't
dereference symlinks, since the kernel only checks the rightmost element of
the path. Depending on the privilege level of the QEMU process, a guest can
end up opening, renaming, changing ACLs, unlinking... files on the host
filesystem.

A possible fix is to always walk paths manually with openat(O_NOFOLLOW), and
use "*at()" variants of all syscalls in the "local" backend code. This will
likely not improve performances for path-based syscalls in the guest, but I
don't see how to fix the issue without kernel support (like an O_PATHSTATIC
flag to tell the full path should not traverse any symlink for example).

A fair amount of code is shared by all security models: this series hence
starts with preparatory patches to split the code. This allows to have
patches of reasonable size, that don't affect too many code paths.

TODO:
- the accesses to metadata files of the "mapped-file" security mode also need
  to be converted

---

Greg Kurz (36):
      9pfs: local: move xattr security ops to 9p-xattr.c
      9pfs: local: split chmod operation per security model
      9pfs: local: split mknod operation per security model
      9pfs: local: split mkdir operation per security model
      9pfs: local: split open2 operation per security model
      9pfs: local: split symlink operation per security model
      9pfs: local: split mkdir operation per security model
      9pfs: local: improve error handling in link op
      9pfs: local: post link operation for mapped-file security
      v9fs: local: improve error handling in rename op
      9pfs: local: post rename operation for mapped-file security
      9pfs: local: pre remove operation for mapped-file security
      9pfs: local: pre unlikat operation for mapped-file security
      9pfs: remove side-effects in local_init()
      9pfs: remove side-effects in local_open() and local_opendir()
      9pfs: introduce openat_nofollow() helper
      9pfs: local: keep a file descriptor on the shared folder
      9pfs: local: open/opendir: don't follow symlinks
      9pfs: local: utimensat: don't follow symlinks
      9pfs: local: readlink: don't follow symlinks
      9pfs: local: truncate: don't follow symlinks
      9pfs: local: statfs: don't follow symlinks
      9pfs: local: mknod/mkdir/open2: don't follow symlinks
      9pfs: local: chmod: don't follow symlinks
      9pfs: local: symlink: don't follow symlinks
      9pfs: local: chown: don't follow symlinks
      9pfs: local: link: don't follow symlinks
      9pfs: local: rename: don't follow symlinks
      9pfs: local: remove: don't follow symlinks
      9pfs: local: unlinkat: don't follow symlinks
      9pfs: local: introduce symlink-attack safe xattr helpers
      9pfs: local: lstat: don't follow symlinks
      9pfs: local: lgetxattr: don't follow symlinks
      9pfs: local: llistxattr: don't follow symlinks
      9pfs: local: lsetxattr: don't follow symlinks
      9pfs: local: lremovexattr: don't follow symlinks


 hw/9pfs/9p-local.c      | 1319 +++++++++++++++++++++++++++++++++--------------
 hw/9pfs/9p-local.h      |   22 +
 hw/9pfs/9p-posix-acl.c  |   48 --
 hw/9pfs/9p-util.c       |   69 ++
 hw/9pfs/9p-util.h       |   25 +
 hw/9pfs/9p-xattr-user.c |   28 -
 hw/9pfs/9p-xattr.c      |  229 ++++++++
 hw/9pfs/9p-xattr.h      |   91 +--
 hw/9pfs/Makefile.objs   |    2 
 9 files changed, 1306 insertions(+), 527 deletions(-)
 create mode 100644 hw/9pfs/9p-local.h
 create mode 100644 hw/9pfs/9p-util.c
 create mode 100644 hw/9pfs/9p-util.h

--
Greg

[Qemu-devel] [PATCH RFC 01/36] 9pfs: local: move xattr security ops to 9p-xattr.c

[Greg Kurz]

These functions are always called indirectly. It really doesn't make sense
for them to sit in a header file.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-xattr.c |   61 ++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-xattr.h |   80 +++++++++-------------------------------------------
 2 files changed, 75 insertions(+), 66 deletions(-)

diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 5d8595ed932a..19a2daf02f5c 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -143,6 +143,67 @@ int v9fs_remove_xattr(FsContext *ctx,
 
 }
 
+ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size)
+{
+    char *buffer;
+    ssize_t ret;
+
+    buffer = rpath(ctx, path);
+    ret = lgetxattr(buffer, name, value, size);
+    g_free(buffer);
+    return ret;
+}
+
+int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
+                size_t size, int flags)
+{
+    char *buffer;
+    int ret;
+
+    buffer = rpath(ctx, path);
+    ret = lsetxattr(buffer, name, value, size, flags);
+    g_free(buffer);
+    return ret;
+}
+
+int pt_removexattr(FsContext *ctx, const char *path, const char *name)
+{
+    char *buffer;
+    int ret;
+
+    buffer = rpath(ctx, path);
+    ret = lremovexattr(path, name);
+    g_free(buffer);
+    return ret;
+}
+
+ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,
+                        void *value, size_t size)
+{
+    errno = ENOTSUP;
+    return -1;
+}
+
+int notsup_setxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size, int flags)
+{
+    errno = ENOTSUP;
+    return -1;
+}
+
+ssize_t notsup_listxattr(FsContext *ctx, const char *path, char *name,
+                         void *value, size_t size)
+{
+    return 0;
+}
+
+int notsup_removexattr(FsContext *ctx, const char *path, const char *name)
+{
+    errno = ENOTSUP;
+    return -1;
+}
+
 XattrOperations *mapped_xattr_ops[] = {
     &mapped_user_xattr,
     &mapped_pacl_xattr,
diff --git a/hw/9pfs/9p-xattr.h b/hw/9pfs/9p-xattr.h
index a853ea641c0b..3f43f5153f3c 100644
--- a/hw/9pfs/9p-xattr.h
+++ b/hw/9pfs/9p-xattr.h
@@ -49,73 +49,21 @@ ssize_t v9fs_list_xattr(FsContext *ctx, const char *path, void *value,
 int v9fs_set_xattr(FsContext *ctx, const char *path, const char *name,
                           void *value, size_t size, int flags);
 int v9fs_remove_xattr(FsContext *ctx, const char *path, const char *name);
+
 ssize_t pt_listxattr(FsContext *ctx, const char *path, char *name, void *value,
                      size_t size);
-
-static inline ssize_t pt_getxattr(FsContext *ctx, const char *path,
-                                  const char *name, void *value, size_t size)
-{
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, name, value, size);
-    g_free(buffer);
-    return ret;
-}
-
-static inline int pt_setxattr(FsContext *ctx, const char *path,
-                              const char *name, void *value,
-                              size_t size, int flags)
-{
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, name, value, size, flags);
-    g_free(buffer);
-    return ret;
-}
-
-static inline int pt_removexattr(FsContext *ctx,
-                                 const char *path, const char *name)
-{
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lremovexattr(path, name);
-    g_free(buffer);
-    return ret;
-}
-
-static inline ssize_t notsup_getxattr(FsContext *ctx, const char *path,
-                                      const char *name, void *value,
-                                      size_t size)
-{
-    errno = ENOTSUP;
-    return -1;
-}
-
-static inline int notsup_setxattr(FsContext *ctx, const char *path,
-                                  const char *name, void *value,
-                                  size_t size, int flags)
-{
-    errno = ENOTSUP;
-    return -1;
-}
-
-static inline ssize_t notsup_listxattr(FsContext *ctx, const char *path,
-                                       char *name, void *value, size_t size)
-{
-    return 0;
-}
-
-static inline int notsup_removexattr(FsContext *ctx,
-                                     const char *path, const char *name)
-{
-    errno = ENOTSUP;
-    return -1;
-}
+ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size);
+int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
+                size_t size, int flags);
+int pt_removexattr(FsContext *ctx, const char *path, const char *name);
+
+ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,
+                        void *value, size_t size);
+int notsup_setxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size, int flags);
+ssize_t notsup_listxattr(FsContext *ctx, const char *path, char *name,
+                         void *value, size_t size);
+int notsup_removexattr(FsContext *ctx, const char *path, const char *name);
 
 #endif

[Qemu-devel] [PATCH RFC 02/36] 9pfs: local: split chmod operation per security model

[Greg Kurz]

Having all security models implemented in one monolithic function is
cumbersome. Especially when the need arises to fix something in the
shared code, as it forces to change all the paths at the same time.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   46 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7de07e1ba67f..73a20657f1fc 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -461,25 +461,53 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
     return ret;
 }
 
-static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
+static int local_chmod_mapped(FsContext *fs_ctx, V9fsPath *fs_path,
+                              FsCred *credp)
+{
+    char *buffer;
+    int ret = -1;
+    char *path = fs_path->data;
+
+    buffer = rpath(fs_ctx, path);
+    ret = local_set_xattr(buffer, credp);
+    g_free(buffer);
+
+    return ret;
+}
+
+static int local_chmod_passthrough(FsContext *fs_ctx, V9fsPath *fs_path,
+                                   FsCred *credp)
 {
     char *buffer;
     int ret = -1;
     char *path = fs_path->data;
 
+    buffer = rpath(fs_ctx, path);
+    ret = chmod(buffer, credp->fc_mode);
+    g_free(buffer);
+
+    return ret;
+}
+
+static int local_chmod_mapped_file(FsContext *fs_ctx, V9fsPath *fs_path,
+                                   FsCred *credp)
+{
+    char *path = fs_path->data;
+
+    return local_set_mapped_file_attr(fs_ctx, path, credp);
+}
+
+static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
+{
     if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        ret = local_set_xattr(buffer, credp);
-        g_free(buffer);
+        return local_chmod_mapped(fs_ctx, fs_path, credp);
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        return local_set_mapped_file_attr(fs_ctx, path, credp);
+        return local_chmod_mapped_file(fs_ctx, fs_path, credp);
     } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
                (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        ret = chmod(buffer, credp->fc_mode);
-        g_free(buffer);
+        return local_chmod_passthrough(fs_ctx, fs_path, credp);
     }
-    return ret;
+    g_assert_not_reached();
 }
 
 static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,

[Qemu-devel] [PATCH RFC 03/36] 9pfs: local: split mknod operation per security model

[Greg Kurz]

Having all security models implemented in one monolithic function is
cumbersome. Especially when the need arises to fix something in the
shared code, as it forces to change all the paths at the same time.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  129 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 92 insertions(+), 37 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 73a20657f1fc..7f513c5728f6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -510,8 +510,8 @@ static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
     g_assert_not_reached();
 }
 
-static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
-                       const char *name, FsCred *credp)
+static int local_mknod_mapped(FsContext *fs_ctx, V9fsPath *dir_path,
+                              const char *name, FsCred *credp)
 {
     char *path;
     int err = -1;
@@ -523,42 +523,49 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
     v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
     path = fullname.data;
 
-    /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
-        if (err == -1) {
-            goto out;
-        }
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+    buffer = rpath(fs_ctx, path);
+    err = mknod(buffer, SM_LOCAL_MODE_BITS | S_IFREG, 0);
+    if (err == -1) {
+        goto out;
+    }
+    err = local_set_xattr(buffer, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    goto out;
 
-        buffer = rpath(fs_ctx, path);
-        err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
-        if (err == -1) {
-            goto out;
-        }
-        err = local_set_mapped_file_attr(fs_ctx, path, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        err = mknod(buffer, credp->fc_mode, credp->fc_rdev);
-        if (err == -1) {
-            goto out;
-        }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
+err_end:
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_mknod_mapped_file(FsContext *fs_ctx, V9fsPath *dir_path,
+                                   const char *name, FsCred *credp)
+{
+    char *path;
+    int err = -1;
+    int serrno = 0;
+    V9fsString fullname;
+    char *buffer = NULL;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    path = fullname.data;
+
+    buffer = rpath(fs_ctx, path);
+    err = mknod(buffer, SM_LOCAL_MODE_BITS | S_IFREG, 0);
+    if (err == -1) {
+        goto out;
+    }
+    err = local_set_mapped_file_attr(fs_ctx, path, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
     }
     goto out;
 
@@ -571,6 +578,54 @@ out:
     return err;
 }
 
+static int local_mknod_passthrough(FsContext *fs_ctx, V9fsPath *dir_path,
+                                   const char *name, FsCred *credp)
+{
+    char *path;
+    int err = -1;
+    int serrno = 0;
+    V9fsString fullname;
+    char *buffer = NULL;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    path = fullname.data;
+
+    buffer = rpath(fs_ctx, path);
+    err = mknod(buffer, credp->fc_mode, credp->fc_rdev);
+    if (err == -1) {
+        goto out;
+    }
+    err = local_post_create_passthrough(fs_ctx, path, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    goto out;
+
+err_end:
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
+                       FsCred *credp)
+{
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+        return local_mknod_mapped(fs_ctx, dir_path, name, credp);
+    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        return local_mknod_mapped_file(fs_ctx, dir_path, name, credp);
+    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
+               (fs_ctx->export_flags & V9FS_SM_NONE)) {
+        return local_mknod_passthrough(fs_ctx, dir_path, name, credp);
+    }
+    g_assert_not_reached();
+}
+
 static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path,
                        const char *name, FsCred *credp)
 {

[Qemu-devel] [PATCH RFC 04/36] 9pfs: local: split mkdir operation per security model

[Greg Kurz]

Having all security models implemented in one monolithic function is
cumbersome. Especially when the need arises to fix something in the
shared code, as it forces to change all the paths at the same time.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  134 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 95 insertions(+), 39 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7f513c5728f6..0d6869123094 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -626,8 +626,8 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
     g_assert_not_reached();
 }
 
-static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path,
-                       const char *name, FsCred *credp)
+static int local_mkdir_mapped(FsContext *fs_ctx, V9fsPath *dir_path,
+                              const char *name, FsCred *credp)
 {
     char *path;
     int err = -1;
@@ -639,43 +639,16 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path,
     v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
     path = fullname.data;
 
-    /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
-        if (err == -1) {
-            goto out;
-        }
-        credp->fc_mode = credp->fc_mode|S_IFDIR;
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        buffer = rpath(fs_ctx, path);
-        err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
-        if (err == -1) {
-            goto out;
-        }
-        credp->fc_mode = credp->fc_mode|S_IFDIR;
-        err = local_set_mapped_file_attr(fs_ctx, path, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        err = mkdir(buffer, credp->fc_mode);
-        if (err == -1) {
-            goto out;
-        }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
+    buffer = rpath(fs_ctx, path);
+    err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
+    if (err == -1) {
+        goto out;
+    }
+    credp->fc_mode = credp->fc_mode | S_IFDIR;
+    err = local_set_xattr(buffer, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
     }
     goto out;
 
@@ -688,6 +661,89 @@ out:
     return err;
 }
 
+static int local_mkdir_mapped_file(FsContext *fs_ctx, V9fsPath *dir_path,
+                                   const char *name, FsCred *credp)
+{
+    char *path;
+    int err = -1;
+    int serrno = 0;
+    V9fsString fullname;
+    char *buffer = NULL;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    path = fullname.data;
+
+    buffer = rpath(fs_ctx, path);
+    err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
+    if (err == -1) {
+        goto out;
+    }
+    credp->fc_mode = credp->fc_mode | S_IFDIR;
+    err = local_set_mapped_file_attr(fs_ctx, path, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    goto out;
+
+err_end:
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_mkdir_passthrough(FsContext *fs_ctx, V9fsPath *dir_path,
+                                   const char *name, FsCred *credp)
+{
+    char *path;
+    int err = -1;
+    int serrno = 0;
+    V9fsString fullname;
+    char *buffer = NULL;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    path = fullname.data;
+
+    buffer = rpath(fs_ctx, path);
+    err = mkdir(buffer, credp->fc_mode);
+    if (err == -1) {
+        goto out;
+    }
+    err = local_post_create_passthrough(fs_ctx, path, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    goto out;
+
+err_end:
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
+                       FsCred *credp)
+{
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+        return local_mkdir_mapped(fs_ctx, dir_path, name, credp);
+    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        return local_mkdir_mapped_file(fs_ctx, dir_path, name, credp);
+    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
+               (fs_ctx->export_flags & V9FS_SM_NONE)) {
+        return local_mkdir_passthrough(fs_ctx, dir_path, name, credp);
+    }
+    g_assert_not_reached();
+}
+
 static int local_fstat(FsContext *fs_ctx, int fid_type,
                        V9fsFidOpenState *fs, struct stat *stbuf)
 {

[Qemu-devel] [PATCH RFC 05/36] 9pfs: local: split open2 operation per security model

[Greg Kurz]

Having all security models implemented in one monolithic function is
cumbersome. Especially when the need arises to fix something in the
shared code, as it forces to change all the paths at the same time.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  167 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 123 insertions(+), 44 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 0d6869123094..4deb19ce8775 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -785,8 +785,9 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
     return err;
 }
 
-static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
-                       int flags, FsCred *credp, V9fsFidOpenState *fs)
+static int local_open2_mapped(FsContext *fs_ctx, V9fsPath *dir_path,
+                              const char *name, int flags, FsCred *credp,
+                              V9fsFidOpenState *fs)
 {
     char *path;
     int fd = -1;
@@ -804,48 +805,65 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
     v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
     path = fullname.data;
 
-    /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
-        }
-        credp->fc_mode = credp->fc_mode|S_IFREG;
-        /* Set cleint credentials in xattr */
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
-        }
-        credp->fc_mode = credp->fc_mode|S_IFREG;
-        /* Set client credentials in .virtfs_metadata directory files */
-        err = local_set_mapped_file_attr(fs_ctx, path, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, flags, credp->fc_mode);
-        if (fd == -1) {
-            err = fd;
-            goto out;
-        }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
+    buffer = rpath(fs_ctx, path);
+    fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
+    if (fd == -1) {
+        err = fd;
+        goto out;
+    }
+    credp->fc_mode = credp->fc_mode | S_IFREG;
+    /* Set cleint credentials in xattr */
+    err = local_set_xattr(buffer, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    err = fd;
+    fs->fd = fd;
+    goto out;
+
+err_end:
+    close(fd);
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_open2_mapped_file(FsContext *fs_ctx, V9fsPath *dir_path,
+                                   const char *name, int flags, FsCred *credp,
+                                   V9fsFidOpenState *fs)
+{
+    char *path;
+    int fd = -1;
+    int err = -1;
+    int serrno = 0;
+    V9fsString fullname;
+    char *buffer = NULL;
+
+    /*
+     * Mark all the open to not follow symlinks
+     */
+    flags |= O_NOFOLLOW;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    path = fullname.data;
+
+    buffer = rpath(fs_ctx, path);
+    fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
+    if (fd == -1) {
+        err = fd;
+        goto out;
+    }
+    credp->fc_mode = credp->fc_mode | S_IFREG;
+    /* Set client credentials in .virtfs_metadata directory files */
+    err = local_set_mapped_file_attr(fs_ctx, path, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
     }
     err = fd;
     fs->fd = fd;
@@ -861,6 +879,67 @@ out:
     return err;
 }
 
+static int local_open2_passthrough(FsContext *fs_ctx, V9fsPath *dir_path,
+                                   const char *name, int flags, FsCred *credp,
+                                   V9fsFidOpenState *fs)
+{
+    char *path;
+    int fd = -1;
+    int err = -1;
+    int serrno = 0;
+    V9fsString fullname;
+    char *buffer = NULL;
+
+    /*
+     * Mark all the open to not follow symlinks
+     */
+    flags |= O_NOFOLLOW;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    path = fullname.data;
+
+    buffer = rpath(fs_ctx, path);
+    fd = open(buffer, flags, credp->fc_mode);
+    if (fd == -1) {
+        err = fd;
+        goto out;
+    }
+    err = local_post_create_passthrough(fs_ctx, path, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    err = fd;
+    fs->fd = fd;
+    goto out;
+
+err_end:
+    close(fd);
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
+                       int flags, FsCred *credp, V9fsFidOpenState *fs)
+{
+    /* Determine the security model */
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+        return local_open2_mapped(fs_ctx, dir_path, name, flags, credp, fs);
+    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        return local_open2_mapped_file(fs_ctx, dir_path, name, flags, credp,
+                                       fs);
+    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
+               (fs_ctx->export_flags & V9FS_SM_NONE)) {
+        return local_open2_passthrough(fs_ctx, dir_path, name, flags, credp,
+                                       fs);
+    }
+    g_assert_not_reached();
+}
 
 static int local_symlink(FsContext *fs_ctx, const char *oldpath,
                          V9fsPath *dir_path, const char *name, FsCred *credp)

[Qemu-devel] [PATCH RFC 06/36] 9pfs: local: split symlink operation per security model

[Greg Kurz]

Having all security models implemented in one monolithic function is
cumbersome. Especially when the need arises to fix something in the
shared code, as it forces to change all the paths at the same time.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  210 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 137 insertions(+), 73 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 4deb19ce8775..f9abd4229b17 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -941,96 +941,144 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
     g_assert_not_reached();
 }
 
-static int local_symlink(FsContext *fs_ctx, const char *oldpath,
-                         V9fsPath *dir_path, const char *name, FsCred *credp)
+static int local_symlink_mapped(FsContext *fs_ctx, const char *oldpath,
+                                V9fsPath *dir_path, const char *name,
+                                FsCred *credp)
 {
     int err = -1;
     int serrno = 0;
     char *newpath;
     V9fsString fullname;
     char *buffer = NULL;
+    int fd;
+    ssize_t oldpath_size, write_size;
 
     v9fs_string_init(&fullname);
     v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
     newpath = fullname.data;
 
-    /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        int fd;
-        ssize_t oldpath_size, write_size;
-        buffer = rpath(fs_ctx, newpath);
-        fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
-        }
-        /* Write the oldpath (target) to the file. */
-        oldpath_size = strlen(oldpath);
-        do {
-            write_size = write(fd, (void *)oldpath, oldpath_size);
-        } while (write_size == -1 && errno == EINTR);
+    buffer = rpath(fs_ctx, newpath);
+    fd = open(buffer, O_CREAT | O_EXCL | O_RDWR | O_NOFOLLOW,
+              SM_LOCAL_MODE_BITS);
+    if (fd == -1) {
+        err = fd;
+        goto out;
+    }
+    /* Write the oldpath (target) to the file. */
+    oldpath_size = strlen(oldpath);
+    do {
+        write_size = write(fd, (void *)oldpath, oldpath_size);
+    } while (write_size == -1 && errno == EINTR);
 
-        if (write_size != oldpath_size) {
-            serrno = errno;
-            close(fd);
-            err = -1;
-            goto err_end;
-        }
+    if (write_size != oldpath_size) {
+        serrno = errno;
         close(fd);
-        /* Set cleint credentials in symlink's xattr */
-        credp->fc_mode = credp->fc_mode|S_IFLNK;
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        int fd;
-        ssize_t oldpath_size, write_size;
-        buffer = rpath(fs_ctx, newpath);
-        fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
-        }
-        /* Write the oldpath (target) to the file. */
-        oldpath_size = strlen(oldpath);
-        do {
-            write_size = write(fd, (void *)oldpath, oldpath_size);
-        } while (write_size == -1 && errno == EINTR);
+        err = -1;
+        goto err_end;
+    }
+    close(fd);
+    /* Set cleint credentials in symlink's xattr */
+    credp->fc_mode = credp->fc_mode | S_IFLNK;
+    err = local_set_xattr(buffer, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    goto out;
 
-        if (write_size != oldpath_size) {
-            serrno = errno;
-            close(fd);
-            err = -1;
-            goto err_end;
-        }
+err_end:
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_symlink_mapped_file(FsContext *fs_ctx, const char *oldpath,
+                                     V9fsPath *dir_path, const char *name,
+                                     FsCred *credp)
+{
+    int err = -1;
+    int serrno = 0;
+    char *newpath;
+    V9fsString fullname;
+    char *buffer = NULL;
+    int fd;
+    ssize_t oldpath_size, write_size;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    newpath = fullname.data;
+
+    buffer = rpath(fs_ctx, newpath);
+    fd = open(buffer, O_CREAT | O_EXCL | O_RDWR | O_NOFOLLOW,
+              SM_LOCAL_MODE_BITS);
+    if (fd == -1) {
+        err = fd;
+        goto out;
+    }
+    /* Write the oldpath (target) to the file. */
+    oldpath_size = strlen(oldpath);
+    do {
+        write_size = write(fd, (void *)oldpath, oldpath_size);
+    } while (write_size == -1 && errno == EINTR);
+
+    if (write_size != oldpath_size) {
+        serrno = errno;
         close(fd);
-        /* Set cleint credentials in symlink's xattr */
-        credp->fc_mode = credp->fc_mode|S_IFLNK;
-        err = local_set_mapped_file_attr(fs_ctx, newpath, credp);
-        if (err == -1) {
+        err = -1;
+        goto err_end;
+    }
+    close(fd);
+    /* Set cleint credentials in symlink's xattr */
+    credp->fc_mode = credp->fc_mode | S_IFLNK;
+    err = local_set_mapped_file_attr(fs_ctx, newpath, credp);
+    if (err == -1) {
+        serrno = errno;
+        goto err_end;
+    }
+    goto out;
+
+err_end:
+    remove(buffer);
+    errno = serrno;
+out:
+    g_free(buffer);
+    v9fs_string_free(&fullname);
+    return err;
+}
+
+static int local_symlink_passthrough(FsContext *fs_ctx, const char *oldpath,
+                                     V9fsPath *dir_path, const char *name,
+                                     FsCred *credp)
+{
+    int err = -1;
+    int serrno = 0;
+    char *newpath;
+    V9fsString fullname;
+    char *buffer = NULL;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    newpath = fullname.data;
+
+    buffer = rpath(fs_ctx, newpath);
+    err = symlink(oldpath, buffer);
+    if (err) {
+        goto out;
+    }
+    err = lchown(buffer, credp->fc_uid, credp->fc_gid);
+    if (err == -1) {
+        /*
+         * If we fail to change ownership and if we are
+         * using security model none. Ignore the error
+         */
+        if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
             serrno = errno;
             goto err_end;
-        }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, newpath);
-        err = symlink(oldpath, buffer);
-        if (err) {
-            goto out;
-        }
-        err = lchown(buffer, credp->fc_uid, credp->fc_gid);
-        if (err == -1) {
-            /*
-             * If we fail to change ownership and if we are
-             * using security model none. Ignore the error
-             */
-            if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
-                serrno = errno;
-                goto err_end;
-            } else
-                err = 0;
+        } else {
+            err = 0;
         }
     }
     goto out;
@@ -1044,6 +1092,22 @@ out:
     return err;
 }
 
+static int local_symlink(FsContext *fs_ctx, const char *oldpath,
+                         V9fsPath *dir_path, const char *name, FsCred *credp)
+{
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+        return local_symlink_mapped(fs_ctx, oldpath, dir_path, name, credp);
+    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        return local_symlink_mapped_file(fs_ctx, oldpath, dir_path, name,
+                                         credp);
+    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
+               (fs_ctx->export_flags & V9FS_SM_NONE)) {
+        return local_symlink_passthrough(fs_ctx, oldpath, dir_path, name,
+                                         credp);
+    }
+    g_assert_not_reached();
+}
+
 static int local_link(FsContext *ctx, V9fsPath *oldpath,
                       V9fsPath *dirpath, const char *name)
 {

[Qemu-devel] [PATCH RFC 07/36] 9pfs: local: split mkdir operation per security model

[Greg Kurz]

Having all security models implemented in one monolithic function is
cumbersome. Especially when the need arises to fix something in the
shared code, as it forces to change all the paths at the same time.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index f9abd4229b17..d424d8994779 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1187,26 +1187,52 @@ static int local_rename(FsContext *ctx, const char *oldpath,
     return err;
 }
 
-static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
+static int local_chown_passthrough(FsContext *fs_ctx, V9fsPath *fs_path,
+                                   FsCred *credp)
 {
     char *buffer;
     int ret = -1;
     char *path = fs_path->data;
 
+    buffer = rpath(fs_ctx, path);
+    ret = lchown(buffer, credp->fc_uid, credp->fc_gid);
+    g_free(buffer);
+    return ret;
+}
+
+static int local_chown_mapped(FsContext *fs_ctx, V9fsPath *fs_path,
+                              FsCred *credp)
+{
+    char *buffer;
+    int ret = -1;
+    char *path = fs_path->data;
+
+    buffer = rpath(fs_ctx, path);
+    ret = local_set_xattr(buffer, credp);
+    g_free(buffer);
+    return ret;
+}
+
+static int local_chown_mapped_file(FsContext *fs_ctx, V9fsPath *fs_path,
+                                   FsCred *credp)
+{
+    char *path = fs_path->data;
+
+    return local_set_mapped_file_attr(fs_ctx, path, credp);
+}
+
+static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
+{
     if ((credp->fc_uid == -1 && credp->fc_gid == -1) ||
         (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
         (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        ret = lchown(buffer, credp->fc_uid, credp->fc_gid);
-        g_free(buffer);
+        return local_chown_passthrough(fs_ctx, fs_path, credp);
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        ret = local_set_xattr(buffer, credp);
-        g_free(buffer);
+        return local_chown_mapped(fs_ctx, fs_path, credp);
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        return local_set_mapped_file_attr(fs_ctx, path, credp);
+        return local_chown_mapped_file(fs_ctx, fs_path, credp);
     }
-    return ret;
+    g_assert_not_reached();
 }
 
 static int local_utimensat(FsContext *s, V9fsPath *fs_path,

[Qemu-devel] [PATCH RFC 08/36] 9pfs: local: improve error handling in link op

[Greg Kurz]

When using the mapped-file security model, we also have to create a link
for the metadata file if it exists. In case of failuire, we should rollback.

That's what this patch does.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index d424d8994779..c0b1907f7901 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1114,6 +1114,7 @@ static int local_link(FsContext *ctx, V9fsPath *oldpath,
     int ret;
     V9fsString newpath;
     char *buffer, *buffer1;
+    int serrno;
 
     v9fs_string_init(&newpath);
     v9fs_string_sprintf(&newpath, "%s/%s", dirpath->data, name);
@@ -1122,25 +1123,36 @@ static int local_link(FsContext *ctx, V9fsPath *oldpath,
     buffer1 = rpath(ctx, newpath.data);
     ret = link(buffer, buffer1);
     g_free(buffer);
-    g_free(buffer1);
+    if (ret < 0) {
+        goto out;
+    }
 
     /* now link the virtfs_metadata files */
-    if (!ret && (ctx->export_flags & V9FS_SM_MAPPED_FILE)) {
+    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        char *vbuffer, *vbuffer1;
+
         /* Link the .virtfs_metadata files. Create the metada directory */
         ret = local_create_mapped_attr_dir(ctx, newpath.data);
         if (ret < 0) {
             goto err_out;
         }
-        buffer = local_mapped_attr_path(ctx, oldpath->data);
-        buffer1 = local_mapped_attr_path(ctx, newpath.data);
-        ret = link(buffer, buffer1);
-        g_free(buffer);
-        g_free(buffer1);
+        vbuffer = local_mapped_attr_path(ctx, oldpath->data);
+        vbuffer1 = local_mapped_attr_path(ctx, newpath.data);
+        ret = link(vbuffer, vbuffer1);
+        g_free(vbuffer);
+        g_free(vbuffer1);
         if (ret < 0 && errno != ENOENT) {
             goto err_out;
         }
     }
+    goto out;
+
 err_out:
+    serrno = errno;
+    remove(buffer1);
+    errno = serrno;
+out:
+    g_free(buffer1);
     v9fs_string_free(&newpath);
     return ret;
 }

[Qemu-devel] [PATCH RFC 09/36] 9pfs: local: post link operation for mapped-file security

[Greg Kurz]

The link operation is really the same for the passthrough and mapped
security models. This patch simply moves the mapped-file bits to a
separate function. This will make future modifications easier.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c0b1907f7901..ebc3e208efa0 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1108,6 +1108,35 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath,
     g_assert_not_reached();
 }
 
+static int local_post_link_mapped_file(FsContext *ctx, V9fsPath *oldpath,
+                                       V9fsPath *dirpath, const char *name)
+{
+    int ret;
+    V9fsString newpath;
+    char *buffer, *buffer1;
+
+    v9fs_string_init(&newpath);
+    v9fs_string_sprintf(&newpath, "%s/%s", dirpath->data, name);
+
+    /* Link the .virtfs_metadata files. Create the metada directory */
+    ret = local_create_mapped_attr_dir(ctx, newpath.data);
+    if (ret < 0) {
+        goto out;
+    }
+    buffer = local_mapped_attr_path(ctx, oldpath->data);
+    buffer1 = local_mapped_attr_path(ctx, newpath.data);
+    ret = link(buffer, buffer1);
+    g_free(buffer);
+    g_free(buffer1);
+    if (ret < 0 && errno == ENOENT) {
+        ret = 0;
+    }
+
+out:
+    v9fs_string_free(&newpath);
+    return ret;
+}
+
 static int local_link(FsContext *ctx, V9fsPath *oldpath,
                       V9fsPath *dirpath, const char *name)
 {
@@ -1129,21 +1158,10 @@ static int local_link(FsContext *ctx, V9fsPath *oldpath,
 
     /* now link the virtfs_metadata files */
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        char *vbuffer, *vbuffer1;
-
-        /* Link the .virtfs_metadata files. Create the metada directory */
-        ret = local_create_mapped_attr_dir(ctx, newpath.data);
+        ret = local_post_link_mapped_file(ctx, oldpath, dirpath, name);
         if (ret < 0) {
             goto err_out;
         }
-        vbuffer = local_mapped_attr_path(ctx, oldpath->data);
-        vbuffer1 = local_mapped_attr_path(ctx, newpath.data);
-        ret = link(vbuffer, vbuffer1);
-        g_free(vbuffer);
-        g_free(vbuffer1);
-        if (ret < 0 && errno != ENOENT) {
-            goto err_out;
-        }
     }
     goto out;
 

[Qemu-devel] [PATCH RFC 10/36] v9fs: local: improve error handling in rename op

[Greg Kurz]

When using the mapped-file security model, we also have to rename the
metadata file if it exists. In case of failure, we should rollback.

To achieve that, this patch moves the renaming of the main file before
the renaming of the metadata file.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index ebc3e208efa0..df453414c902 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1192,26 +1192,39 @@ static int local_rename(FsContext *ctx, const char *oldpath,
 {
     int err;
     char *buffer, *buffer1;
+    int serrno;
+
+    buffer = rpath(ctx, oldpath);
+    buffer1 = rpath(ctx, newpath);
+    err = rename(buffer, buffer1);
+    if (err < 0) {
+        goto out;
+    }
 
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        char *vbuffer, *vbuffer1;
+
         err = local_create_mapped_attr_dir(ctx, newpath);
         if (err < 0) {
-            return err;
+            goto out_err;
         }
         /* rename the .virtfs_metadata files */
-        buffer = local_mapped_attr_path(ctx, oldpath);
-        buffer1 = local_mapped_attr_path(ctx, newpath);
-        err = rename(buffer, buffer1);
-        g_free(buffer);
-        g_free(buffer1);
+        vbuffer = local_mapped_attr_path(ctx, oldpath);
+        vbuffer1 = local_mapped_attr_path(ctx, newpath);
+        err = rename(vbuffer, vbuffer1);
+        g_free(vbuffer);
+        g_free(vbuffer1);
         if (err < 0 && errno != ENOENT) {
-            return err;
+            goto err_out;
         }
     }
+    goto out;
 
-    buffer = rpath(ctx, oldpath);
-    buffer1 = rpath(ctx, newpath);
-    err = rename(buffer, buffer1);
+err_out:
+    serrno = errno;
+    rename(buffer1, buffer);
+    errno = serrno;
+out:
     g_free(buffer);
     g_free(buffer1);
     return err;

[Qemu-devel] [PATCH RFC 11/36] 9pfs: local: post rename operation for mapped-file security

[Greg Kurz]

The rename operation is really the same for the passthrough and mapped
security models. This patch simply moves the mapped-file bits to a
separate function. This will make future modifications easier.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index df453414c902..74953e4dbfe0 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1187,6 +1187,29 @@ static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
     return ret;
 }
 
+static int local_post_rename_mapped_file(FsContext *ctx, const char *oldpath,
+                                         const char *newpath)
+{
+    int err;
+    char *buffer, *buffer1;
+
+    err = local_create_mapped_attr_dir(ctx, newpath);
+    if (err < 0) {
+        return err;
+    }
+    /* rename the .virtfs_metadata files */
+    buffer = local_mapped_attr_path(ctx, oldpath);
+    buffer1 = local_mapped_attr_path(ctx, newpath);
+    err = rename(buffer, buffer1);
+    g_free(buffer);
+    g_free(buffer1);
+    if (err < 0 && errno != ENOENT) {
+        return err;
+    }
+
+    return 0;
+}
+
 static int local_rename(FsContext *ctx, const char *oldpath,
                         const char *newpath)
 {
@@ -1202,19 +1225,8 @@ static int local_rename(FsContext *ctx, const char *oldpath,
     }
 
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        char *vbuffer, *vbuffer1;
-
-        err = local_create_mapped_attr_dir(ctx, newpath);
+        err = local_post_rename_mapped_file(ctx, oldpath, newpath);
         if (err < 0) {
-            goto out_err;
-        }
-        /* rename the .virtfs_metadata files */
-        vbuffer = local_mapped_attr_path(ctx, oldpath);
-        vbuffer1 = local_mapped_attr_path(ctx, newpath);
-        err = rename(vbuffer, vbuffer1);
-        g_free(vbuffer);
-        g_free(vbuffer1);
-        if (err < 0 && errno != ENOENT) {
             goto err_out;
         }
     }

[Qemu-devel] [PATCH RFC 12/36] 9pfs: local: pre remove operation for mapped-file security

[Greg Kurz]

The remove operation is really the same for the passthrough and mapped
security models. This patch simply moves the mapped-file bits to a
separate function. This will make future modifications easier.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   74 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 30 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 74953e4dbfe0..7506d2155c05 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1303,41 +1303,25 @@ static int local_utimensat(FsContext *s, V9fsPath *fs_path,
     return ret;
 }
 
-static int local_remove(FsContext *ctx, const char *path)
+static int local_pre_remove_mapped_file(FsContext *ctx, const char *path)
 {
     int err;
     struct stat stbuf;
     char *buffer;
 
-    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        buffer = rpath(ctx, path);
-        err =  lstat(buffer, &stbuf);
-        g_free(buffer);
-        if (err) {
-            goto err_out;
-        }
-        /*
-         * If directory remove .virtfs_metadata contained in the
-         * directory
-         */
-        if (S_ISDIR(stbuf.st_mode)) {
-            buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root,
-                                     path, VIRTFS_META_DIR);
-            err = remove(buffer);
-            g_free(buffer);
-            if (err < 0 && errno != ENOENT) {
-                /*
-                 * We didn't had the .virtfs_metadata file. May be file created
-                 * in non-mapped mode ?. Ignore ENOENT.
-                 */
-                goto err_out;
-            }
-        }
-        /*
-         * Now remove the name from parent directory
-         * .virtfs_metadata directory
-         */
-        buffer = local_mapped_attr_path(ctx, path);
+    buffer = rpath(ctx, path);
+    err =  lstat(buffer, &stbuf);
+    g_free(buffer);
+    if (err) {
+        goto err_out;
+    }
+    /*
+     * If directory remove .virtfs_metadata contained in the
+     * directory
+     */
+    if (S_ISDIR(stbuf.st_mode)) {
+        buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root,
+                                 path, VIRTFS_META_DIR);
         err = remove(buffer);
         g_free(buffer);
         if (err < 0 && errno != ENOENT) {
@@ -1348,6 +1332,36 @@ static int local_remove(FsContext *ctx, const char *path)
             goto err_out;
         }
     }
+    /*
+     * Now remove the name from parent directory
+     * .virtfs_metadata directory
+     */
+    buffer = local_mapped_attr_path(ctx, path);
+    err = remove(buffer);
+    g_free(buffer);
+    if (err < 0 && errno == ENOENT) {
+        /*
+         * We didn't had the .virtfs_metadata file. May be file created
+         * in non-mapped mode ?. Ignore ENOENT.
+         */
+        err = 0;
+    }
+
+err_out:
+    return err;
+}
+
+static int local_remove(FsContext *ctx, const char *path)
+{
+    int err;
+    char *buffer;
+
+    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        err =  local_pre_remove_mapped_file(ctx, path);
+        if (err) {
+            goto err_out;
+        }
+    }
 
     buffer = rpath(ctx, path);
     err = remove(buffer);

[Qemu-devel] [PATCH RFC 13/36] 9pfs: local: pre unlikat operation for mapped-file security

[Greg Kurz]

The unlinkat operation is really the same for the passthrough and
mapped security models. This patch simply moves the mapped-file bits
to a separate function. This will make future modifications easier.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   68 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7506d2155c05..b75ad452decc 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1462,39 +1462,23 @@ static int local_renameat(FsContext *ctx, V9fsPath *olddir,
     return ret;
 }
 
-static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
-                          const char *name, int flags)
+static int local_pre_unlinkat_mapped_file(FsContext *ctx, V9fsPath *dir,
+                                          const char *name, int flags)
 {
     int ret;
     V9fsString fullname;
     char *buffer;
 
     v9fs_string_init(&fullname);
-
     v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name);
-    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        if (flags == AT_REMOVEDIR) {
-            /*
-             * If directory remove .virtfs_metadata contained in the
-             * directory
-             */
-            buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root,
-                                     fullname.data, VIRTFS_META_DIR);
-            ret = remove(buffer);
-            g_free(buffer);
-            if (ret < 0 && errno != ENOENT) {
-                /*
-                 * We didn't had the .virtfs_metadata file. May be file created
-                 * in non-mapped mode ?. Ignore ENOENT.
-                 */
-                goto err_out;
-            }
-        }
+
+    if (flags == AT_REMOVEDIR) {
         /*
-         * Now remove the name from parent directory
-         * .virtfs_metadata directory.
+         * If directory remove .virtfs_metadata contained in the
+         * directory
          */
-        buffer = local_mapped_attr_path(ctx, fullname.data);
+        buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root,
+                                 fullname.data, VIRTFS_META_DIR);
         ret = remove(buffer);
         g_free(buffer);
         if (ret < 0 && errno != ENOENT) {
@@ -1505,6 +1489,42 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
             goto err_out;
         }
     }
+    /*
+     * Now remove the name from parent directory
+     * .virtfs_metadata directory.
+     */
+    buffer = local_mapped_attr_path(ctx, fullname.data);
+    ret = remove(buffer);
+    g_free(buffer);
+    if (ret < 0 && errno == ENOENT) {
+        /*
+         * We didn't had the .virtfs_metadata file. May be file created
+         * in non-mapped mode ?. Ignore ENOENT.
+         */
+        ret = 0;
+    }
+
+err_out:
+    v9fs_string_free(&fullname);
+    return ret;
+}
+
+static int local_unlinkat(FsContext *ctx, V9fsPath *dir, const char *name,
+                          int flags)
+{
+    int ret;
+    V9fsString fullname;
+    char *buffer;
+
+    v9fs_string_init(&fullname);
+    v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name);
+
+    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        ret = local_pre_unlinkat_mapped_file(ctx, dir, name, flags);
+        if (ret < 0) {
+            goto err_out;
+        }
+    }
     /* Remove the name finally */
     buffer = rpath(ctx, fullname.data);
     ret = remove(buffer);

[Qemu-devel] [PATCH RFC 14/36] 9pfs: remove side-effects in local_init()

[Greg Kurz]

If this function fails, it should not modify *ctx.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b75ad452decc..443197841780 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1565,9 +1565,25 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
 
 static int local_init(FsContext *ctx)
 {
-    int err = 0;
     struct statfs stbuf;
 
+#ifdef FS_IOC_GETVERSION
+    /*
+     * use ioc_getversion only if the iocl is definied
+     */
+    if (statfs(ctx->fs_root, &stbuf) < 0) {
+        return -1;
+    }
+    switch (stbuf.f_type) {
+    case EXT2_SUPER_MAGIC:
+    case BTRFS_SUPER_MAGIC:
+    case REISERFS_SUPER_MAGIC:
+    case XFS_SUPER_MAGIC:
+        ctx->exops.get_st_gen = local_ioc_getversion;
+        break;
+    }
+#endif
+
     if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
         ctx->xops = passthrough_xattr_ops;
     } else if (ctx->export_flags & V9FS_SM_MAPPED) {
@@ -1582,23 +1598,8 @@ static int local_init(FsContext *ctx)
         ctx->xops = passthrough_xattr_ops;
     }
     ctx->export_flags |= V9FS_PATHNAME_FSCONTEXT;
-#ifdef FS_IOC_GETVERSION
-    /*
-     * use ioc_getversion only if the iocl is definied
-     */
-    err = statfs(ctx->fs_root, &stbuf);
-    if (!err) {
-        switch (stbuf.f_type) {
-        case EXT2_SUPER_MAGIC:
-        case BTRFS_SUPER_MAGIC:
-        case REISERFS_SUPER_MAGIC:
-        case XFS_SUPER_MAGIC:
-            ctx->exops.get_st_gen = local_ioc_getversion;
-            break;
-        }
-    }
-#endif
-    return err;
+
+    return 0;
 }
 
 static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)

[Qemu-devel] [PATCH RFC 15/36] 9pfs: remove side-effects in local_open() and local_opendir()

[Greg Kurz]

If these functions fail, they should not change *fs. Let's use local
variables to fix this. While here, let's also do some cosmetic fixes
on the function args.

This doesn't fix any bug, it is just preparatory cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 443197841780..d3c6ccf30b53 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -351,30 +351,37 @@ static int local_closedir(FsContext *ctx, V9fsFidOpenState *fs)
     return closedir(fs->dir.stream);
 }
 
-static int local_open(FsContext *ctx, V9fsPath *fs_path,
-                      int flags, V9fsFidOpenState *fs)
+static int local_open(FsContext *ctx, V9fsPath *fs_path, int flags,
+                      V9fsFidOpenState *fs)
 {
     char *buffer;
     char *path = fs_path->data;
+    int fd;
 
     buffer = rpath(ctx, path);
-    fs->fd = open(buffer, flags | O_NOFOLLOW);
+    fd = open(buffer, flags | O_NOFOLLOW);
     g_free(buffer);
+    if (fd == -1) {
+        return -1;
+    }
+    fs->fd = fd;
     return fs->fd;
 }
 
-static int local_opendir(FsContext *ctx,
-                         V9fsPath *fs_path, V9fsFidOpenState *fs)
+static int local_opendir(FsContext *ctx, V9fsPath *fs_path,
+                         V9fsFidOpenState *fs)
 {
     char *buffer;
     char *path = fs_path->data;
+    DIR *stream;
 
     buffer = rpath(ctx, path);
-    fs->dir.stream = opendir(buffer);
+    stream = opendir(buffer);
     g_free(buffer);
-    if (!fs->dir.stream) {
+    if (!stream) {
         return -1;
     }
+    fs->dir.stream = stream;
     return 0;
 }
 

[Qemu-devel] [PATCH RFC 16/36] 9pfs: introduce openat_nofollow() helper

[Greg Kurz]

When using the passthrough security mode, symbolic links created by the
guest are actual symbolic links on the host file system.

Since the resolution of symbolic links during path walk is supposed to
occur on the client side. The server should never have to follow them.
The current code hence relies on O_NOFOLLOW and "l*()" variants of system
calls. Unfortunately, this only applies to the rightmost path component.
A guest could maliciously replace any component in a trusted path with a
symbolic link. This could allow any guest to escape a virtfs shared folder.

This patch introduces a variant of the openat() syscall that successively
opens each path element with O_NOFOLLOW. It will be used by subsequent
patches to implement symlink-safe path walk for any access to the backend.

Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-util.c     |   69 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-util.h     |   25 ++++++++++++++++++
 hw/9pfs/Makefile.objs |    2 +
 3 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 hw/9pfs/9p-util.c
 create mode 100644 hw/9pfs/9p-util.h

diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
new file mode 100644
index 000000000000..48292d948401
--- /dev/null
+++ b/hw/9pfs/9p-util.c
@@ -0,0 +1,69 @@
+/*
+ * 9p utilities
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ *  Greg Kurz <groug@kaod.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "9p-util.h"
+
+int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode)
+{
+    const char *tail = path;
+    const char *c;
+    int fd;
+
+    fd = dup(dirfd);
+    if (fd == -1) {
+        return -1;
+    }
+
+    while (*tail) {
+        int next_fd;
+        char *head;
+
+        while (*tail == '/') {
+            tail++;
+        }
+
+        if (!*tail) {
+            break;
+        }
+
+        head = g_strdup(tail);
+        c = strchr(tail, '/');
+        if (c) {
+            head[c - tail] = 0;
+            next_fd = openat(fd, head, O_DIRECTORY | O_RDONLY | O_NOFOLLOW);
+        } else {
+            /* We don't want bad things to happen like opening a file that
+             * sits outside the virtfs export, or hanging on a named pipe,
+             * or changing the controlling process of a terminal.
+             */
+            flags |= O_NOFOLLOW | O_NONBLOCK | O_NOCTTY;
+            next_fd = openat(fd, head, flags, mode);
+        }
+        g_free(head);
+        if (next_fd == -1) {
+            close_preserve_errno(fd);
+            return -1;
+        }
+        close(fd);
+        fd = next_fd;
+
+        if (!c) {
+            break;
+        }
+        tail = c + 1;
+    }
+    /* O_NONBLOCK was only needed to open the file. Let's drop it. */
+    assert(!fcntl(fd, F_SETFL, flags));
+
+    return fd;
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
new file mode 100644
index 000000000000..e19673d85222
--- /dev/null
+++ b/hw/9pfs/9p-util.h
@@ -0,0 +1,25 @@
+/*
+ * 9p utilities
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ *  Greg Kurz <groug@kaod.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_9P_UTIL_H
+#define QEMU_9P_UTIL_H
+
+static inline void close_preserve_errno(int fd)
+{
+    int serrno = errno;
+    close(fd);
+    errno = serrno;
+}
+
+int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode);
+
+#endif
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index da0ae0cfdbae..32197e6671dd 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-y  = 9p.o
+common-obj-y  = 9p.o 9p-util.o
 common-obj-y += 9p-local.o 9p-xattr.o
 common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
 common-obj-y += coth.o cofs.o codir.o cofile.o

[Qemu-devel] [PATCH RFC 17/36] 9pfs: local: keep a file descriptor on the shared folder

[Greg Kurz]

This patch opens the shared folder and caches the file descriptor, so that
it can be used to do symlink-safe path walk. Since nothing prevents several
QEMU instances to pass overlapping export paths to -fsdev, we also make
sure that the export path doesn't traverse a symlink either.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index d3c6ccf30b53..8a1d52cd6c2a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "9p.h"
 #include "9p-xattr.h"
+#include "9p-util.h"
 #include "fsdev/qemu-fsdev.h"   /* local_ops */
 #include <arpa/inet.h>
 #include <pwd.h>
@@ -43,6 +44,10 @@
 #define BTRFS_SUPER_MAGIC 0x9123683E
 #endif
 
+struct local_data {
+    int mountfd;
+};
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -1573,13 +1578,28 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
 static int local_init(FsContext *ctx)
 {
     struct statfs stbuf;
+    struct local_data *data = g_malloc(sizeof(*data));
+    int rootfd;
+
+    rootfd = open("/", O_DIRECTORY | O_RDONLY);
+    if (rootfd == -1) {
+        goto err;
+    }
+
+    data->mountfd = openat_nofollow(rootfd, ctx->fs_root,
+                                    O_DIRECTORY | O_RDONLY, 0);
+    close_preserve_errno(rootfd);
+    if (data->mountfd == -1) {
+        goto err;
+    }
 
 #ifdef FS_IOC_GETVERSION
     /*
      * use ioc_getversion only if the iocl is definied
      */
-    if (statfs(ctx->fs_root, &stbuf) < 0) {
-        return -1;
+    if (fstatfs(data->mountfd, &stbuf) < 0) {
+        close_preserve_errno(data->mountfd);
+        goto err;
     }
     switch (stbuf.f_type) {
     case EXT2_SUPER_MAGIC:
@@ -1606,7 +1626,20 @@ static int local_init(FsContext *ctx)
     }
     ctx->export_flags |= V9FS_PATHNAME_FSCONTEXT;
 
+    ctx->private = data;
     return 0;
+
+err:
+    g_free(data);
+    return -1;
+}
+
+static void local_cleanup(FsContext *ctx)
+{
+    struct local_data *data = ctx->private;
+
+    close(data->mountfd);
+    g_free(data);
 }
 
 static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
@@ -1649,6 +1682,7 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
 FileOperations local_ops = {
     .parse_opts = local_parse_opts,
     .init  = local_init,
+    .cleanup = local_cleanup,
     .lstat = local_lstat,
     .readlink = local_readlink,
     .close = local_close,

[Qemu-devel] [PATCH RFC 18/36] 9pfs: local: open/opendir: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   31 +++++++++++++++++++++----------
 hw/9pfs/9p-local.h |   20 ++++++++++++++++++++
 2 files changed, 41 insertions(+), 10 deletions(-)
 create mode 100644 hw/9pfs/9p-local.h

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 8a1d52cd6c2a..783b4006ffd4 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "9p.h"
+#include "9p-local.h"
 #include "9p-xattr.h"
 #include "9p-util.h"
 #include "fsdev/qemu-fsdev.h"   /* local_ops */
@@ -48,6 +49,18 @@ struct local_data {
     int mountfd;
 };
 
+int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
+                        mode_t mode)
+{
+    struct local_data *data = fs_ctx->private;
+    return openat_nofollow(data->mountfd, path, flags, mode);
+}
+
+int local_opendir_nofollow(FsContext *fs_ctx, const char *path)
+{
+    return local_open_nofollow(fs_ctx, path, O_DIRECTORY | O_RDONLY, 0);
+}
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -359,13 +372,9 @@ static int local_closedir(FsContext *ctx, V9fsFidOpenState *fs)
 static int local_open(FsContext *ctx, V9fsPath *fs_path, int flags,
                       V9fsFidOpenState *fs)
 {
-    char *buffer;
-    char *path = fs_path->data;
     int fd;
 
-    buffer = rpath(ctx, path);
-    fd = open(buffer, flags | O_NOFOLLOW);
-    g_free(buffer);
+    fd = local_open_nofollow(ctx, fs_path->data, flags, 0);
     if (fd == -1) {
         return -1;
     }
@@ -376,13 +385,15 @@ static int local_open(FsContext *ctx, V9fsPath *fs_path, int flags,
 static int local_opendir(FsContext *ctx, V9fsPath *fs_path,
                          V9fsFidOpenState *fs)
 {
-    char *buffer;
-    char *path = fs_path->data;
+    int dirfd;
     DIR *stream;
 
-    buffer = rpath(ctx, path);
-    stream = opendir(buffer);
-    g_free(buffer);
+    dirfd = local_opendir_nofollow(ctx, fs_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
+
+    stream = fdopendir(dirfd);
     if (!stream) {
         return -1;
     }
diff --git a/hw/9pfs/9p-local.h b/hw/9pfs/9p-local.h
new file mode 100644
index 000000000000..32c72749d9df
--- /dev/null
+++ b/hw/9pfs/9p-local.h
@@ -0,0 +1,20 @@
+/*
+ * 9p local backend utilities
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ *  Greg Kurz <groug@kaod.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_9P_LOCAL_H
+#define QEMU_9P_LOCAL_H
+
+int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
+                        mode_t mode);
+int local_opendir_nofollow(FsContext *fs_ctx, const char *path);
+
+#endif

[Qemu-devel] [PATCH RFC 19/36] 9pfs: local: utimensat: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   37 ++++++++++++++++++++++++++++++-------
 hw/9pfs/9p-local.h |    2 ++
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 783b4006ffd4..a1fff04c3219 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -61,6 +61,22 @@ int local_opendir_nofollow(FsContext *fs_ctx, const char *path)
     return local_open_nofollow(fs_ctx, path, O_DIRECTORY | O_RDONLY, 0);
 }
 
+char *local_dirname(const char *path)
+{
+    char *tmp_path = g_strdup(path);
+    char *result = g_strdup(dirname(tmp_path));
+    g_free(tmp_path);
+    return result;
+}
+
+char *local_basename(const char *path)
+{
+    char *tmp_path = g_strdup(path);
+    char *result = g_strdup(basename(tmp_path));
+    g_free(tmp_path);
+    return result;
+}
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -1313,16 +1329,23 @@ static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
     g_assert_not_reached();
 }
 
-static int local_utimensat(FsContext *s, V9fsPath *fs_path,
+static int local_utimensat(FsContext *fs_ctx, V9fsPath *fs_path,
                            const struct timespec *buf)
 {
-    char *buffer;
-    int ret;
-    char *path = fs_path->data;
+    char *dirpath = local_dirname(fs_path->data);
+    char *name = local_basename(fs_path->data);
+    int dirfd, ret = -1;
 
-    buffer = rpath(s, path);
-    ret = qemu_utimens(buffer, buf);
-    g_free(buffer);
+    dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(name);
     return ret;
 }
 
diff --git a/hw/9pfs/9p-local.h b/hw/9pfs/9p-local.h
index 32c72749d9df..2732da2b5d55 100644
--- a/hw/9pfs/9p-local.h
+++ b/hw/9pfs/9p-local.h
@@ -16,5 +16,7 @@
 int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
                         mode_t mode);
 int local_opendir_nofollow(FsContext *fs_ctx, const char *path);
+char *local_dirname(const char *path);
+char *local_basename(const char *path);
 
 #endif

[Qemu-devel] [PATCH RFC 20/36] 9pfs: local: readlink: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index a1fff04c3219..1f9239de07e5 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -346,31 +346,48 @@ err:
     return -1;
 }
 
+static int readlink_nofollow(FsContext *fs_ctx, const char *path, char *buf,
+                             size_t bufsz)
+{
+    char *dirpath = local_dirname(path);
+    char *name = local_basename(path);
+    int dirfd, ret = -1;
+
+    dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = readlinkat(dirfd, name, buf, bufsz);
+    close_preserve_errno(dirfd);
+out:
+    g_free(name);
+    g_free(dirpath);
+    return ret;
+}
+
 static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
                               char *buf, size_t bufsz)
 {
-    ssize_t tsize = -1;
-    char *buffer;
-    char *path = fs_path->data;
+    ssize_t tsize;
 
     if ((fs_ctx->export_flags & V9FS_SM_MAPPED) ||
         (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE)) {
         int fd;
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, O_RDONLY | O_NOFOLLOW);
-        g_free(buffer);
+
+        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
         if (fd == -1) {
             return -1;
         }
         do {
             tsize = read(fd, (void *)buf, bufsz);
         } while (tsize == -1 && errno == EINTR);
-        close(fd);
+        close_preserve_errno(fd);
     } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
                (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        tsize = readlink(buffer, buf, bufsz);
-        g_free(buffer);
+        tsize = readlink_nofollow(fs_ctx, fs_path->data, buf, bufsz);
+    } else {
+        g_assert_not_reached();
     }
     return tsize;
 }

[Qemu-devel] [PATCH RFC 21/36] 9pfs: local: truncate: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 1f9239de07e5..4377aa6524c2 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1231,15 +1231,16 @@ out:
     return ret;
 }
 
-static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
+static int local_truncate(FsContext *fs_ctx, V9fsPath *fs_path, off_t size)
 {
-    char *buffer;
-    int ret;
-    char *path = fs_path->data;
+    int fd, ret;
 
-    buffer = rpath(ctx, path);
-    ret = truncate(buffer, size);
-    g_free(buffer);
+    fd = local_open_nofollow(fs_ctx, fs_path->data, O_WRONLY, 0);
+    if (fd == -1) {
+        return -1;
+    }
+    ret = ftruncate(fd, size);
+    close_preserve_errno(fd);
     return ret;
 }
 

[Qemu-devel] [PATCH RFC 22/36] 9pfs: local: statfs: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 4377aa6524c2..dbc56b16979c 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1452,15 +1452,14 @@ static int local_fsync(FsContext *ctx, int fid_type,
     }
 }
 
-static int local_statfs(FsContext *s, V9fsPath *fs_path, struct statfs *stbuf)
+static int local_statfs(FsContext *fs_ctx, V9fsPath *fs_path,
+                        struct statfs *stbuf)
 {
-    char *buffer;
-    int ret;
-    char *path = fs_path->data;
+    int fd, ret;
 
-    buffer = rpath(s, path);
-    ret = statfs(buffer, stbuf);
-    g_free(buffer);
+    fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
+    ret = fstatfs(fd, stbuf);
+    close_preserve_errno(fd);
     return ret;
 }
 

[Qemu-devel] [PATCH RFC 23/36] 9pfs: local: mknod/mkdir/open2: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" security model.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  128 ++++++++++++++++++++++++----------------------------
 1 file changed, 59 insertions(+), 69 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index dbc56b16979c..48d46b6abd28 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -77,6 +77,13 @@ char *local_basename(const char *path)
     return result;
 }
 
+static void unlinkat_preserve_errno(int dirfd, const char *path, int flags)
+{
+    int serrno = errno;
+    unlinkat(dirfd, path, flags);
+    errno = serrno;
+}
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -319,31 +326,41 @@ static int local_set_xattr(const char *path, FsCred *credp)
     return 0;
 }
 
-static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
-                                         FsCred *credp)
+static int local_post_create_passthrough(FsContext *fs_ctx, int dirfd,
+                                         const char *name, FsCred *credp)
 {
-    char *buffer;
+    int fd, err = -1;
 
-    buffer = rpath(fs_ctx, path);
-    if (lchown(buffer, credp->fc_uid, credp->fc_gid) < 0) {
+    if (fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+                 AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH) < 0) {
         /*
          * If we fail to change ownership and if we are
          * using security model none. Ignore the error
          */
         if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
-            goto err;
+            return -1;
         }
     }
 
-    if (chmod(buffer, credp->fc_mode & 07777) < 0) {
-        goto err;
+    if (name[0]) {
+        fd = openat_nofollow(dirfd, name, O_RDONLY, 0);
+        if (fd == -1) {
+            return -1;
+        }
+    } else {
+        fd = dirfd;
     }
 
-    g_free(buffer);
-    return 0;
-err:
-    g_free(buffer);
-    return -1;
+    if (fchmod(fd, credp->fc_mode & 07777) < 0) {
+        goto out;
+    }
+    err = 0;
+
+out:
+    if (name[0]) {
+        close_preserve_errno(fd);
+    }
+    return err;
 }
 
 static int readlink_nofollow(FsContext *fs_ctx, const char *path, char *buf,
@@ -637,34 +654,27 @@ out:
 static int local_mknod_passthrough(FsContext *fs_ctx, V9fsPath *dir_path,
                                    const char *name, FsCred *credp)
 {
-    char *path;
-    int err = -1;
-    int serrno = 0;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd, err;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    path = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
-    buffer = rpath(fs_ctx, path);
-    err = mknod(buffer, credp->fc_mode, credp->fc_rdev);
+    err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
     if (err == -1) {
         goto out;
     }
-    err = local_post_create_passthrough(fs_ctx, path, credp);
+    err = local_post_create_passthrough(fs_ctx, dirfd, name, credp);
     if (err == -1) {
-        serrno = errno;
         goto err_end;
     }
     goto out;
 
 err_end:
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name, 0);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 
@@ -755,34 +765,27 @@ out:
 static int local_mkdir_passthrough(FsContext *fs_ctx, V9fsPath *dir_path,
                                    const char *name, FsCred *credp)
 {
-    char *path;
-    int err = -1;
-    int serrno = 0;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd, err;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    path = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
-    buffer = rpath(fs_ctx, path);
-    err = mkdir(buffer, credp->fc_mode);
+    err = mkdirat(dirfd, name, credp->fc_mode);
     if (err == -1) {
         goto out;
     }
-    err = local_post_create_passthrough(fs_ctx, path, credp);
+    err = local_post_create_passthrough(fs_ctx, dirfd, name, credp);
     if (err == -1) {
-        serrno = errno;
         goto err_end;
     }
     goto out;
 
 err_end:
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name, AT_REMOVEDIR);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 
@@ -939,44 +942,31 @@ static int local_open2_passthrough(FsContext *fs_ctx, V9fsPath *dir_path,
                                    const char *name, int flags, FsCred *credp,
                                    V9fsFidOpenState *fs)
 {
-    char *path;
-    int fd = -1;
     int err = -1;
-    int serrno = 0;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd, fd;
 
-    /*
-     * Mark all the open to not follow symlinks
-     */
-    flags |= O_NOFOLLOW;
-
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    path = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
-    buffer = rpath(fs_ctx, path);
-    fd = open(buffer, flags, credp->fc_mode);
+    fd = openat(dirfd, name, flags | O_NOFOLLOW, credp->fc_mode);
     if (fd == -1) {
-        err = fd;
         goto out;
     }
-    err = local_post_create_passthrough(fs_ctx, path, credp);
+    err = local_post_create_passthrough(fs_ctx, fd, "", credp);
     if (err == -1) {
-        serrno = errno;
         goto err_end;
     }
-    err = fd;
     fs->fd = fd;
     goto out;
 
 err_end:
-    close(fd);
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name,
+                            flags & O_DIRECTORY ? AT_REMOVEDIR : 0);
+    close_preserve_errno(fd);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 

[Qemu-devel] [PATCH RFC 24/36] 9pfs: local: chmod: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" security model.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 48d46b6abd28..9dfa3e306245 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -551,13 +551,14 @@ static int local_chmod_mapped(FsContext *fs_ctx, V9fsPath *fs_path,
 static int local_chmod_passthrough(FsContext *fs_ctx, V9fsPath *fs_path,
                                    FsCred *credp)
 {
-    char *buffer;
-    int ret = -1;
-    char *path = fs_path->data;
+    int fd, ret;
 
-    buffer = rpath(fs_ctx, path);
-    ret = chmod(buffer, credp->fc_mode);
-    g_free(buffer);
+    fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
+    if (fd == -1) {
+        return -1;
+    }
+    ret = fchmod(fd, credp->fc_mode);
+    close_preserve_errno(fd);
 
     return ret;
 }

[Qemu-devel] [PATCH RFC 25/36] 9pfs: local: symlink: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" security model.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 9dfa3e306245..bbc08184564f 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1100,29 +1100,25 @@ static int local_symlink_passthrough(FsContext *fs_ctx, const char *oldpath,
                                      V9fsPath *dir_path, const char *name,
                                      FsCred *credp)
 {
-    int err = -1;
-    int serrno = 0;
-    char *newpath;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd, err;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    newpath = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
-    buffer = rpath(fs_ctx, newpath);
-    err = symlink(oldpath, buffer);
+    err = symlinkat(oldpath, dirfd, name);
     if (err) {
         goto out;
     }
-    err = lchown(buffer, credp->fc_uid, credp->fc_gid);
+    err = fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+                   AT_SYMLINK_NOFOLLOW);
     if (err == -1) {
         /*
          * If we fail to change ownership and if we are
          * using security model none. Ignore the error
          */
         if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
-            serrno = errno;
             goto err_end;
         } else {
             err = 0;
@@ -1131,11 +1127,9 @@ static int local_symlink_passthrough(FsContext *fs_ctx, const char *oldpath,
     goto out;
 
 err_end:
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name, 0);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 

[Qemu-devel] [PATCH RFC 26/36] 9pfs: local: chown: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" security model.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index bbc08184564f..5e320917c484 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1287,13 +1287,21 @@ out:
 static int local_chown_passthrough(FsContext *fs_ctx, V9fsPath *fs_path,
                                    FsCred *credp)
 {
-    char *buffer;
-    int ret = -1;
-    char *path = fs_path->data;
+    char *dirpath = local_dirname(fs_path->data);
+    char *name = local_basename(fs_path->data);
+    int dirfd, ret = -1;
 
-    buffer = rpath(fs_ctx, path);
-    ret = lchown(buffer, credp->fc_uid, credp->fc_gid);
-    g_free(buffer);
+    dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+    ret = fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+                   AT_SYMLINK_NOFOLLOW);
+    close_preserve_errno(dirfd);
+
+out:
+    g_free(name);
+    g_free(dirpath);
     return ret;
 }
 

[Qemu-devel] [PATCH RFC 27/36] 9pfs: local: link: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" and "mapped" security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 5e320917c484..de860db3d70b 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1181,21 +1181,23 @@ out:
 static int local_link(FsContext *ctx, V9fsPath *oldpath,
                       V9fsPath *dirpath, const char *name)
 {
-    int ret;
-    V9fsString newpath;
-    char *buffer, *buffer1;
-    int serrno;
-
-    v9fs_string_init(&newpath);
-    v9fs_string_sprintf(&newpath, "%s/%s", dirpath->data, name);
+    char *odirpath = local_dirname(oldpath->data);
+    char *oname = local_basename(oldpath->data);
+    int ret = -1;
+    int odirfd, ndirfd;
 
-    buffer = rpath(ctx, oldpath->data);
-    buffer1 = rpath(ctx, newpath.data);
-    ret = link(buffer, buffer1);
-    g_free(buffer);
-    if (ret < 0) {
+    odirfd = local_opendir_nofollow(ctx, odirpath);
+    if (odirfd == -1) {
         goto out;
     }
+    ndirfd = local_opendir_nofollow(ctx, dirpath->data);
+    if (ndirfd == -1) {
+        goto out_close_odirfd;
+    }
+    ret = linkat(odirfd, oname, ndirfd, name, AT_SYMLINK_FOLLOW);
+    if (ret < 0) {
+        goto out_close_ndirfd;
+    }
 
     /* now link the virtfs_metadata files */
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
@@ -1204,15 +1206,17 @@ static int local_link(FsContext *ctx, V9fsPath *oldpath,
             goto err_out;
         }
     }
-    goto out;
+    goto out_close_ndirfd;
 
 err_out:
-    serrno = errno;
-    remove(buffer1);
-    errno = serrno;
+    unlinkat_preserve_errno(ndirfd, name, 0);
+out_close_ndirfd:
+    close_preserve_errno(ndirfd);
+out_close_odirfd:
+    close_preserve_errno(odirfd);
 out:
-    g_free(buffer1);
-    v9fs_string_free(&newpath);
+    g_free(oname);
+    g_free(odirpath);
     return ret;
 }
 

[Qemu-devel] [PATCH RFC 28/36] 9pfs: local: rename: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" and "mapped" security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index de860db3d70b..364da435350b 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -84,6 +84,14 @@ static void unlinkat_preserve_errno(int dirfd, const char *path, int flags)
     errno = serrno;
 }
 
+static void renameat_preserve_errno(int odirfd, const char *opath, int ndirfd,
+                                    const char *npath)
+{
+    int serrno = errno;
+    renameat(odirfd, opath, ndirfd, npath);
+    errno = serrno;
+}
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -1259,16 +1267,22 @@ static int local_post_rename_mapped_file(FsContext *ctx, const char *oldpath,
 static int local_rename(FsContext *ctx, const char *oldpath,
                         const char *newpath)
 {
-    int err;
-    char *buffer, *buffer1;
-    int serrno;
+    char *odirpath = local_dirname(oldpath);
+    char *oname = local_basename(oldpath);
+    char *ndirpath = local_dirname(newpath);
+    char *nname = local_basename(newpath);
+    int err = -1;
+    int odirfd, ndirfd;
 
-    buffer = rpath(ctx, oldpath);
-    buffer1 = rpath(ctx, newpath);
-    err = rename(buffer, buffer1);
-    if (err < 0) {
+    odirfd = local_opendir_nofollow(ctx, odirpath);
+    if (odirfd == -1) {
         goto out;
     }
+    ndirfd = local_opendir_nofollow(ctx, ndirpath);
+    if (ndirfd < 0) {
+        goto out_close_odirfd;
+    }
+    err = renameat(odirfd, oname, ndirfd, nname);
 
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
         err = local_post_rename_mapped_file(ctx, oldpath, newpath);
@@ -1276,15 +1290,19 @@ static int local_rename(FsContext *ctx, const char *oldpath,
             goto err_out;
         }
     }
-    goto out;
+    goto out_close_ndirfd;
 
 err_out:
-    serrno = errno;
-    rename(buffer1, buffer);
-    errno = serrno;
+    renameat_preserve_errno(ndirfd, nname, odirfd, oname);
+out_close_ndirfd:
+    close_preserve_errno(ndirfd);
+out_close_odirfd:
+    close_preserve_errno(odirfd);
 out:
-    g_free(buffer);
-    g_free(buffer1);
+    g_free(nname);
+    g_free(ndirpath);
+    g_free(oname);
+    g_free(odirpath);
     return err;
 }
 

[Qemu-devel] [PATCH RFC 29/36] 9pfs: local: remove: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" and "mapped" security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 364da435350b..573852a55a00 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1432,8 +1432,17 @@ err_out:
 
 static int local_remove(FsContext *ctx, const char *path)
 {
-    int err;
-    char *buffer;
+    char *dirpath = local_dirname(path);
+    char *name = local_basename(path);
+    struct stat stbuf;
+    int flags = 0;
+    int err = -1;
+    int dirfd;
+
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd) {
+        goto out;
+    }
 
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
         err =  local_pre_remove_mapped_file(ctx, path);
@@ -1442,10 +1451,19 @@ static int local_remove(FsContext *ctx, const char *path)
         }
     }
 
-    buffer = rpath(ctx, path);
-    err = remove(buffer);
-    g_free(buffer);
+    if (fstatat(dirfd, path, &stbuf, AT_SYMLINK_NOFOLLOW) < 0) {
+        goto err_out;
+    }
+    if (S_ISDIR(stbuf.st_mode)) {
+        flags |= AT_REMOVEDIR;
+    }
+
+    err = unlinkat(dirfd, name, flags);
 err_out:
+    close_preserve_errno(dirfd);
+out:
+    g_free(name);
+    g_free(dirpath);
     return err;
 }
 

[Qemu-devel] [PATCH RFC 30/36] 9pfs: local: unlinkat: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" and "mapped" security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 573852a55a00..60edfb25f8a5 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1609,25 +1609,23 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, const char *name,
                           int flags)
 {
     int ret;
-    V9fsString fullname;
-    char *buffer;
+    int dirfd;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name);
+    dirfd = local_opendir_nofollow(ctx, dir->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
         ret = local_pre_unlinkat_mapped_file(ctx, dir, name, flags);
-        if (ret < 0) {
+        if (ret) {
             goto err_out;
         }
     }
-    /* Remove the name finally */
-    buffer = rpath(ctx, fullname.data);
-    ret = remove(buffer);
-    g_free(buffer);
 
+    ret = unlinkat(dirfd, name, flags);
 err_out:
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return ret;
 }
 

[Qemu-devel] [PATCH RFC 31/36] 9pfs: local: introduce symlink-attack safe xattr helpers

[Greg Kurz]

There are no "at" variants for xattr syscalls. This patch implement them
using a separate process.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-xattr.c |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-xattr.h |   11 ++++
 2 files changed, 167 insertions(+)

diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 19a2daf02f5c..ea0695f37242 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -15,7 +15,163 @@
 #include "9p.h"
 #include "fsdev/file-op-9p.h"
 #include "9p-xattr.h"
+#include "9p-util.h"
 
+enum {
+    XATTRAT_OP_GET = 0,
+    XATTRAT_OP_LIST,
+    XATTRAT_OP_SET,
+    XATTRAT_OP_REMOVE
+};
+
+struct xattrat_data {
+    ssize_t ret;
+    int serrno;
+    char value[0];
+};
+
+static void munmap_preserver_errno(void *addr, size_t length)
+{
+    int serrno = errno;
+    munmap(addr, length);
+    errno = serrno;
+}
+
+static ssize_t do_xattrat_op(int op_type, int dirfd, const char *path,
+                             const char *name, void *value, size_t size,
+                             int flags)
+{
+    struct xattrat_data *data;
+    pid_t pid;
+    ssize_t ret = -1;
+    int wstatus;
+
+    data = mmap(NULL, sizeof(*data) + size, PROT_READ | PROT_WRITE,
+                MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+    if (data == MAP_FAILED) {
+        return -1;
+    }
+    data->ret = -1;
+
+    pid = fork();
+    if (pid < 0) {
+        goto err_out;
+    } else if (pid == 0) {
+        if (fchdir(dirfd) == 0) {
+            switch (op_type) {
+            case XATTRAT_OP_GET:
+                data->ret = lgetxattr(path, name, data->value, size);
+                break;
+            case XATTRAT_OP_LIST:
+                data->ret = llistxattr(path, data->value, size);
+                break;
+            case XATTRAT_OP_SET:
+                data->ret = lsetxattr(path, name, data->value, size, flags);
+                break;
+            case XATTRAT_OP_REMOVE:
+                data->ret = lremovexattr(path, name);
+                break;
+            default:
+                g_assert_not_reached();
+            }
+        }
+        data->serrno = errno;
+        _exit(0);
+    }
+    assert(waitpid(pid, &wstatus, 0) == pid && WIFEXITED(wstatus));
+
+    ret = data->ret;
+    if (ret < 0) {
+        errno = data->serrno;
+        goto err_out;
+    }
+    memcpy(value, data->value, data->ret);
+
+err_out:
+    munmap_preserver_errno(data, sizeof(*data) + size);
+    return ret;
+}
+
+ssize_t fgetxattrat(int dirfd, const char *path, const char *name, void *value,
+                    size_t size)
+{
+    return do_xattrat_op(XATTRAT_OP_GET, dirfd, path, name, value, size, 0);
+}
+
+ssize_t local_getxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size)
+{
+    char *dirpath = local_dirname(path);
+    char *filename = local_basename(path);
+    int dirfd;
+    ssize_t ret = -1;
+
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = fgetxattrat(dirfd, filename, name, value, size);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(filename);
+    return ret;
+}
+
+static ssize_t fsetxattrat(int dirfd, const char *path, const char *name,
+                           void *value, size_t size, int flags)
+{
+    return do_xattrat_op(XATTRAT_OP_SET, dirfd, path, name, value, size, flags);
+}
+
+ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size,
+                                int flags)
+{
+    char *dirpath = local_dirname(path);
+    char *filename = local_basename(path);
+    int dirfd;
+    ssize_t ret = -1;
+
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = fsetxattrat(dirfd, filename, name, value, size, flags);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(filename);
+    return ret;
+}
+
+static ssize_t fremovexattrat(int dirfd, const char *path, const char *name)
+{
+    return do_xattrat_op(XATTRAT_OP_GET, dirfd, path, name, NULL, 0, 0);
+}
+
+ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
+                                   const char *name)
+{
+    char *dirpath = local_dirname(path);
+    char *filename = local_basename(path);
+    int dirfd;
+    ssize_t ret = -1;
+
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = fremovexattrat(dirfd, filename, name);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(filename);
+    return ret;
+}
 
 static XattrOperations *get_xattr_operations(XattrOperations **h,
                                              const char *name)
diff --git a/hw/9pfs/9p-xattr.h b/hw/9pfs/9p-xattr.h
index 3f43f5153f3c..d95ccc26a18d 100644
--- a/hw/9pfs/9p-xattr.h
+++ b/hw/9pfs/9p-xattr.h
@@ -15,6 +15,7 @@
 #define QEMU_9P_XATTR_H
 
 #include "qemu/xattr.h"
+#include "9p-local.h"
 
 typedef struct xattr_operations
 {
@@ -29,6 +30,16 @@ typedef struct xattr_operations
                        const char *path, const char *name);
 } XattrOperations;
 
+ssize_t fgetxattrat(int dirfd, const char *path, const char *name, void *value,
+                    size_t size);
+
+ssize_t local_getxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size);
+ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size,
+                                int flags);
+ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
+                                   const char *name);
 
 extern XattrOperations mapped_user_xattr;
 extern XattrOperations passthrough_user_xattr;

[Qemu-devel] [PATCH RFC 32/36] 9pfs: local: lstat: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for the "passthrough" and "mapped" security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 60edfb25f8a5..b46a81792ecf 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -168,12 +168,17 @@ static void local_mapped_file_attr(FsContext *ctx, const char *path,
 
 static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
 {
-    int err;
-    char *buffer;
-    char *path = fs_path->data;
+    int err = -1;
+    char *dirpath = local_dirname(fs_path->data);
+    char *name = local_basename(fs_path->data);
+    int dirfd;
 
-    buffer = rpath(fs_ctx, path);
-    err =  lstat(buffer, stbuf);
+    dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    err = fstatat(dirfd, name, stbuf, AT_SYMLINK_NOFOLLOW);
     if (err) {
         goto err_out;
     }
@@ -183,25 +188,32 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
         gid_t tmp_gid;
         mode_t tmp_mode;
         dev_t tmp_dev;
-        if (getxattr(buffer, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
+
+        if (fgetxattrat(dirfd, name, "user.virtfs.uid", &tmp_uid,
+                        sizeof(uid_t)) > 0) {
             stbuf->st_uid = le32_to_cpu(tmp_uid);
         }
-        if (getxattr(buffer, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
+        if (fgetxattrat(dirfd, name, "user.virtfs.gid", &tmp_gid,
+                        sizeof(gid_t)) > 0) {
             stbuf->st_gid = le32_to_cpu(tmp_gid);
         }
-        if (getxattr(buffer, "user.virtfs.mode",
-                    &tmp_mode, sizeof(mode_t)) > 0) {
+        if (fgetxattrat(dirfd, name, "user.virtfs.mode", &tmp_mode,
+                        sizeof(mode_t)) > 0) {
             stbuf->st_mode = le32_to_cpu(tmp_mode);
         }
-        if (getxattr(buffer, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
+        if (fgetxattrat(dirfd, name, "user.virtfs.rdev", &tmp_dev,
+                        sizeof(dev_t)) > 0) {
             stbuf->st_rdev = le64_to_cpu(tmp_dev);
         }
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        local_mapped_file_attr(fs_ctx, path, stbuf);
+        local_mapped_file_attr(fs_ctx, fs_path->data, stbuf);
     }
 
 err_out:
-    g_free(buffer);
+    close_preserve_errno(dirfd);
+out:
+    g_free(name);
+    g_free(dirpath);
     return err;
 }
 

[Qemu-devel] [PATCH RFC 33/36] 9pfs: local: lgetxattr: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-posix-acl.c  |   16 ++--------------
 hw/9pfs/9p-xattr-user.c |    8 +-------
 hw/9pfs/9p-xattr.c      |    8 +-------
 3 files changed, 4 insertions(+), 28 deletions(-)

diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index ec003181cd33..9435e27a368c 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -25,13 +25,7 @@
 static ssize_t mp_pacl_getxattr(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, MAP_ACL_ACCESS, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, MAP_ACL_ACCESS, value, size);
 }
 
 static ssize_t mp_pacl_listxattr(FsContext *ctx, const char *path,
@@ -89,13 +83,7 @@ static int mp_pacl_removexattr(FsContext *ctx,
 static ssize_t mp_dacl_getxattr(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, MAP_ACL_DEFAULT, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, MAP_ACL_DEFAULT, value, size);
 }
 
 static ssize_t mp_dacl_listxattr(FsContext *ctx, const char *path,
diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index f87530c8b526..4071fbc4c086 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -20,9 +20,6 @@
 static ssize_t mp_user_getxattr(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
     if (strncmp(name, "user.virtfs.", 12) == 0) {
         /*
          * Don't allow fetch of user.virtfs namesapce
@@ -31,10 +28,7 @@ static ssize_t mp_user_getxattr(FsContext *ctx, const char *path,
         errno = ENOATTR;
         return -1;
     }
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, name, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, name, value, size);
 }
 
 static ssize_t mp_user_listxattr(FsContext *ctx, const char *path,
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index ea0695f37242..29f4f940a23f 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -302,13 +302,7 @@ int v9fs_remove_xattr(FsContext *ctx,
 ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
                     void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, name, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, name, value, size);
 }
 
 int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,

[Qemu-devel] [PATCH RFC 34/36] 9pfs: local: llistxattr: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-xattr.c |   30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 29f4f940a23f..08df02e0bab2 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -214,6 +214,11 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
     return name_size;
 }
 
+static ssize_t flistxattrat(int dirfd, const char *path, char *list,
+                            size_t size)
+{
+    return do_xattrat_op(XATTRAT_OP_LIST, dirfd, path, NULL, list, size, 0);
+}
 
 /*
  * Get the list and pass to each layer to find out whether
@@ -223,24 +228,37 @@ ssize_t v9fs_list_xattr(FsContext *ctx, const char *path,
                         void *value, size_t vsize)
 {
     ssize_t size = 0;
-    char *buffer;
     void *ovalue = value;
     XattrOperations *xops;
     char *orig_value, *orig_value_start;
     ssize_t xattr_len, parsed_len = 0, attr_len;
+    char *dirpath, *name;
+    int dirfd;
 
     /* Get the actual len */
-    buffer = rpath(ctx, path);
-    xattr_len = llistxattr(buffer, value, 0);
+    dirpath = local_dirname(path);
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    g_free(dirpath);
+    if (dirfd == -1) {
+        return -1;
+    }
+
+    name = local_basename(path);
+    xattr_len = flistxattrat(dirfd, name, value, 0);
     if (xattr_len <= 0) {
-        g_free(buffer);
+        g_free(name);
+        close_preserve_errno(dirfd);
         return xattr_len;
     }
 
     /* Now fetch the xattr and find the actual size */
     orig_value = g_malloc(xattr_len);
-    xattr_len = llistxattr(buffer, orig_value, xattr_len);
-    g_free(buffer);
+    xattr_len = flistxattrat(dirfd, name, orig_value, xattr_len);
+    g_free(name);
+    close_preserve_errno(dirfd);
+    if (xattr_len < 0) {
+        return -1;
+    }
 
     /* store the orig pointer */
     orig_value_start = orig_value;

[Qemu-devel] [PATCH RFC 35/36] 9pfs: local: lsetxattr: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-posix-acl.c  |   18 ++++--------------
 hw/9pfs/9p-xattr-user.c |    8 +-------
 hw/9pfs/9p-xattr.c      |    8 +-------
 3 files changed, 6 insertions(+), 28 deletions(-)

diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index 9435e27a368c..0154e2a7605f 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -50,13 +50,8 @@ static ssize_t mp_pacl_listxattr(FsContext *ctx, const char *path,
 static int mp_pacl_setxattr(FsContext *ctx, const char *path, const char *name,
                             void *value, size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, MAP_ACL_ACCESS, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, MAP_ACL_ACCESS, value, size,
+                                   flags);
 }
 
 static int mp_pacl_removexattr(FsContext *ctx,
@@ -108,13 +103,8 @@ static ssize_t mp_dacl_listxattr(FsContext *ctx, const char *path,
 static int mp_dacl_setxattr(FsContext *ctx, const char *path, const char *name,
                             void *value, size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, MAP_ACL_DEFAULT, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, MAP_ACL_DEFAULT, value, size,
+                                   flags);
 }
 
 static int mp_dacl_removexattr(FsContext *ctx,
diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index 4071fbc4c086..1840a5db66f3 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -67,9 +67,6 @@ static ssize_t mp_user_listxattr(FsContext *ctx, const char *path,
 static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
                             void *value, size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
     if (strncmp(name, "user.virtfs.", 12) == 0) {
         /*
          * Don't allow fetch of user.virtfs namesapce
@@ -78,10 +75,7 @@ static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
         errno = EACCES;
         return -1;
     }
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, name, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, name, value, size, flags);
 }
 
 static int mp_user_removexattr(FsContext *ctx,
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 08df02e0bab2..053ab6eff692 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -326,13 +326,7 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
 int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
                 size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, name, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, name, value, size, flags);
 }
 
 int pt_removexattr(FsContext *ctx, const char *path, const char *name)

[Qemu-devel] [PATCH RFC 36/36] 9pfs: local: lremovexattr: don't follow symlinks

[Greg Kurz]

This fixes CVE-2016-9602 for all security models.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-posix-acl.c  |   14 ++++----------
 hw/9pfs/9p-xattr-user.c |   12 +++---------
 hw/9pfs/9p-xattr.c      |    8 +-------
 3 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index 0154e2a7605f..73aee0e9d1fa 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -54,14 +54,12 @@ static int mp_pacl_setxattr(FsContext *ctx, const char *path, const char *name,
                                    flags);
 }
 
-static int mp_pacl_removexattr(FsContext *ctx,
-                               const char *path, const char *name)
+static int mp_pacl_removexattr(FsContext *ctx, const char *path,
+                               const char *name)
 {
     int ret;
-    char *buffer;
 
-    buffer = rpath(ctx, path);
-    ret  = lremovexattr(buffer, MAP_ACL_ACCESS);
+    ret = local_removexattr_nofollow(ctx, MAP_ACL_ACCESS, name);
     if (ret == -1 && errno == ENODATA) {
         /*
          * We don't get ENODATA error when trying to remove a
@@ -71,7 +69,6 @@ static int mp_pacl_removexattr(FsContext *ctx,
         errno = 0;
         ret = 0;
     }
-    g_free(buffer);
     return ret;
 }
 
@@ -111,10 +108,8 @@ static int mp_dacl_removexattr(FsContext *ctx,
                                const char *path, const char *name)
 {
     int ret;
-    char *buffer;
 
-    buffer = rpath(ctx, path);
-    ret  = lremovexattr(buffer, MAP_ACL_DEFAULT);
+    ret = local_removexattr_nofollow(ctx, MAP_ACL_DEFAULT, name);
     if (ret == -1 && errno == ENODATA) {
         /*
          * We don't get ENODATA error when trying to remove a
@@ -124,7 +119,6 @@ static int mp_dacl_removexattr(FsContext *ctx,
         errno = 0;
         ret = 0;
     }
-    g_free(buffer);
     return ret;
 }
 
diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index 1840a5db66f3..ec606a4146a7 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -78,12 +78,9 @@ static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
     return local_setxattr_nofollow(ctx, path, name, value, size, flags);
 }
 
-static int mp_user_removexattr(FsContext *ctx,
-                               const char *path, const char *name)
+static int mp_user_removexattr(FsContext *ctx, const char *path,
+                               const char *name)
 {
-    char *buffer;
-    int ret;
-
     if (strncmp(name, "user.virtfs.", 12) == 0) {
         /*
          * Don't allow fetch of user.virtfs namesapce
@@ -92,10 +89,7 @@ static int mp_user_removexattr(FsContext *ctx,
         errno = EACCES;
         return -1;
     }
-    buffer = rpath(ctx, path);
-    ret = lremovexattr(buffer, name);
-    g_free(buffer);
-    return ret;
+    return local_removexattr_nofollow(ctx, path, name);
 }
 
 XattrOperations mapped_user_xattr = {
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 053ab6eff692..bbfef248e767 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -331,13 +331,7 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
 
 int pt_removexattr(FsContext *ctx, const char *path, const char *name)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lremovexattr(path, name);
-    g_free(buffer);
-    return ret;
+    return local_removexattr_nofollow(ctx, path, name);
 }
 
 ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,