From 506f3275829258fdf193bbf0f98f3f9e8c0f3c45 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:13 +0200 Subject: [PATCH 01/11] virtio-9p/xen-9p: move 9p specific bits to core 9p code These bits aren't related to the transport so let's move them to the core code. Signed-off-by: Greg Kurz Reviewed-by: Stefano Stabellini --- hw/9pfs/9p.c | 8 +++++++- hw/9pfs/9p.h | 2 +- hw/9pfs/virtio-9p-device.c | 8 +------- hw/9pfs/xen-9p-backend.c | 6 +----- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index ab3e22f231..b3048371a8 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -3446,12 +3446,16 @@ static inline bool is_read_only_op(V9fsPDU *pdu) } } -void pdu_submit(V9fsPDU *pdu) +void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr) { Coroutine *co; CoroutineEntry *handler; V9fsState *s = pdu->s; + pdu->size = le32_to_cpu(hdr->size_le); + pdu->id = hdr->id; + pdu->tag = le16_to_cpu(hdr->tag_le); + if (pdu->id >= ARRAY_SIZE(pdu_co_handlers) || (pdu_co_handlers[pdu->id] == NULL)) { handler = v9fs_op_not_supp; @@ -3462,6 +3466,8 @@ void pdu_submit(V9fsPDU *pdu) if (is_ro_export(&s->ctx) && !is_read_only_op(pdu)) { handler = v9fs_fs_ro; } + + qemu_co_queue_init(&pdu->complete); co = qemu_coroutine_create(handler, pdu); qemu_coroutine_enter(co); } diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index 5312d8a424..c886ba78d2 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -347,7 +347,7 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...); ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...); V9fsPDU *pdu_alloc(V9fsState *s); void pdu_free(V9fsPDU *pdu); -void pdu_submit(V9fsPDU *pdu); +void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr); void v9fs_reset(V9fsState *s); struct V9fsTransport { diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c index 3782f43702..245abd8aae 100644 --- a/hw/9pfs/virtio-9p-device.c +++ b/hw/9pfs/virtio-9p-device.c @@ -70,13 +70,7 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) goto out_free_req; } - pdu->size = le32_to_cpu(out.size_le); - - pdu->id = out.id; - pdu->tag = le16_to_cpu(out.tag_le); - - qemu_co_queue_init(&pdu->complete); - pdu_submit(pdu); + pdu_submit(pdu, &out); } return; diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c index 5df97c90fa..922cc967be 100644 --- a/hw/9pfs/xen-9p-backend.c +++ b/hw/9pfs/xen-9p-backend.c @@ -243,14 +243,10 @@ static int xen_9pfs_receive(Xen9pfsRing *ring) /* cannot fail, because we only handle one request per ring at a time */ pdu = pdu_alloc(&ring->priv->state); - pdu->size = le32_to_cpu(h.size_le); - pdu->id = h.id; - pdu->tag = le32_to_cpu(h.tag_le); ring->out_size = le32_to_cpu(h.size_le); ring->out_cons = cons + le32_to_cpu(h.size_le); - qemu_co_queue_init(&pdu->complete); - pdu_submit(pdu); + pdu_submit(pdu, &h); return 0; } From 57a0aa6b507050734e302f1ff6da452518bd74e8 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:13 +0200 Subject: [PATCH 02/11] fsdev: don't allow unknown format in marshal/unmarshal The code only uses well known format strings. An unknown format token is a bug. Signed-off-by: Greg Kurz Reviewed-by: Stefano Stabellini --- fsdev/9p-iov-marshal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c index 1d16f8df4b..a1c9beddd2 100644 --- a/fsdev/9p-iov-marshal.c +++ b/fsdev/9p-iov-marshal.c @@ -168,7 +168,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset, break; } default: - break; + g_assert_not_reached(); } if (copied < 0) { return copied; @@ -281,7 +281,7 @@ ssize_t v9fs_iov_vmarshal(struct iovec *in_sg, int in_num, size_t offset, break; } default: - break; + g_assert_not_reached(); } if (copied < 0) { return copied; From a17d8659c466aa2470fdf5b05c88e9e68d40d6ee Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:13 +0200 Subject: [PATCH 03/11] 9pfs: drop pdu_push_and_notify() Only pdu_complete() needs to notify the client that a request has completed. Signed-off-by: Greg Kurz Reviewed-by: Stefano Stabellini --- hw/9pfs/9p.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index b3048371a8..a25d31e62f 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -65,11 +65,6 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...) return ret; } -static void pdu_push_and_notify(V9fsPDU *pdu) -{ - pdu->s->transport->push_and_notify(pdu); -} - static int omode_to_uflags(int8_t mode) { int ret = 0; @@ -668,7 +663,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len) pdu->size = len; pdu->id = id; - pdu_push_and_notify(pdu); + pdu->s->transport->push_and_notify(pdu); /* Now wakeup anybody waiting in flush for this request */ if (!qemu_co_queue_next(&pdu->complete)) { From 6a87e7929f97b86c5823d4616fa1aa7636b2f116 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:13 +0200 Subject: [PATCH 04/11] 9pfs: local: fix unlink of alien files in mapped-file mode When trying to remove a file from a directory, both created in non-mapped mode, the file remains and EBADF is returned to the guest. This is a regression introduced by commit "df4938a6651b 9pfs: local: unlinkat: don't follow symlinks" when fixing CVE-2016-9602. It changed the way we unlink the metadata file from ret = remove("$dir/.virtfs_metadata/$name"); if (ret < 0 && errno != ENOENT) { /* Error out */ } /* Ignore absence of metadata */ to fd = openat("$dir/.virtfs_metadata") unlinkat(fd, "$name") if (ret < 0 && errno != ENOENT) { /* Error out */ } /* Ignore absence of metadata */ If $dir was created in non-mapped mode, openat() fails with ENOENT and we pass -1 to unlinkat(), which fails in turn with EBADF. We just need to check the return of openat() and ignore ENOENT, in order to restore the behaviour we had with remove(). Signed-off-by: Greg Kurz Reviewed-by: Eric Blake [groug: rewrote the comments as suggested by Eric] --- hw/9pfs/9p-local.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index a2486566af..226234d386 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -992,6 +992,14 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, if (ctx->export_flags & V9FS_SM_MAPPED_FILE) { int map_dirfd; + /* We need to remove the metadata as well: + * - the metadata directory if we're removing a directory + * - the metadata file in the parent's metadata directory + * + * If any of these are missing (ie, ENOENT) then we're probably + * trying to remove something that wasn't created in mapped-file + * mode. We just ignore the error. + */ if (flags == AT_REMOVEDIR) { int fd; @@ -999,32 +1007,20 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, if (fd == -1) { goto err_out; } - /* - * If directory remove .virtfs_metadata contained in the - * directory - */ ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR); close_preserve_errno(fd); 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; } } - /* - * Now remove the name from parent directory - * .virtfs_metadata directory. - */ map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); - ret = unlinkat(map_dirfd, name, 0); - close_preserve_errno(map_dirfd); - if (ret < 0 && errno != ENOENT) { - /* - * We didn't had the .virtfs_metadata file. May be file created - * in non-mapped mode ?. Ignore ENOENT. - */ + if (map_dirfd != -1) { + ret = unlinkat(map_dirfd, name, 0); + close_preserve_errno(map_dirfd); + if (ret < 0 && errno != ENOENT) { + goto err_out; + } + } else if (errno != ENOENT) { goto err_out; } } From 4be56c19593c15bc8d16abcb63bfdd4697a5754d Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:13 +0200 Subject: [PATCH 05/11] fsdev: fix virtfs-proxy-helper cwd Since chroot() doesn't change the current directory, it is indeed a good practice to chdir() to the target directory and then then chroot(), or to chroot() to the target directory and then chdir("/"). The current code does neither of them actually. Let's go for the latter. This doesn't fix any security issue since all of this takes place before the helper begins to process requests. Signed-off-by: Greg Kurz Reviewed-by: Eric Blake --- fsdev/virtfs-proxy-helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 54f7ad1c48..4c4238f62e 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -1129,14 +1129,14 @@ int main(int argc, char **argv) } } - if (chdir("/") < 0) { - do_perror("chdir"); - goto error; - } if (chroot(rpath) < 0) { do_perror("chroot"); goto error; } + if (chdir("/") < 0) { + do_perror("chdir"); + goto error; + } get_version = false; #ifdef FS_IOC_GETVERSION From 24df3371d97a7516605aef8abbc253a8c162b211 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:14 +0200 Subject: [PATCH 06/11] 9pfs: assume utimensat() and futimens() are present The utimensat() and futimens() syscalls have been around for ages (ie, glibc 2.6 and linux 2.6.22), and the decision was already taken to switch to utimensat() anyway when fixing CVE-2016-9602 in 2.9. Signed-off-by: Greg Kurz Reviewed-by: Eric Blake --- fsdev/virtfs-proxy-helper.c | 3 ++- hw/9pfs/9p-handle.c | 5 ----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 4c4238f62e..6c066ec9a0 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -945,7 +945,8 @@ static int process_requests(int sock) &spec[0].tv_sec, &spec[0].tv_nsec, &spec[1].tv_sec, &spec[1].tv_nsec); if (retval > 0) { - retval = qemu_utimens(path.data, spec); + retval = utimensat(AT_FDCWD, path.data, spec, + AT_SYMLINK_NOFOLLOW); if (retval < 0) { retval = -errno; } diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c index 1687661bc9..9875f1894c 100644 --- a/hw/9pfs/9p-handle.c +++ b/hw/9pfs/9p-handle.c @@ -378,7 +378,6 @@ static int handle_utimensat(FsContext *ctx, V9fsPath *fs_path, const struct timespec *buf) { int ret; -#ifdef CONFIG_UTIMENSAT int fd; struct handle_data *data = (struct handle_data *)ctx->private; @@ -388,10 +387,6 @@ static int handle_utimensat(FsContext *ctx, V9fsPath *fs_path, } ret = futimens(fd, buf); close(fd); -#else - ret = -1; - errno = ENOSYS; -#endif return ret; } From fcdcf1eed2fd26bfe836080755ba4322d3c1f2cc Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:14 +0200 Subject: [PATCH 07/11] util: drop old utimensat() compat code Now that 9pfs and virtfs-proxy-helper have been converted to utimensat(), we don't need to keep qemu_utimens() anymore. Signed-off-by: Greg Kurz Reviewed-by: Eric Blake --- configure | 22 ------------------ include/sysemu/os-posix.h | 11 --------- util/oslib-posix.c | 47 --------------------------------------- 3 files changed, 80 deletions(-) diff --git a/configure b/configure index 1a5ee4b909..0586ec9c64 100755 --- a/configure +++ b/configure @@ -3629,25 +3629,6 @@ if compile_prog "" "" ; then inotify1=yes fi -# check if utimensat and futimens are supported -utimens=no -cat > $TMPC << EOF -#define _ATFILE_SOURCE -#include -#include -#include - -int main(void) -{ - utimensat(AT_FDCWD, "foo", NULL, 0); - futimens(0, NULL); - return 0; -} -EOF -if compile_prog "" "" ; then - utimens=yes -fi - # check if pipe2 is there pipe2=no cat > $TMPC << EOF @@ -5434,9 +5415,6 @@ fi if test "$curses" = "yes" ; then echo "CONFIG_CURSES=y" >> $config_host_mak fi -if test "$utimens" = "yes" ; then - echo "CONFIG_UTIMENSAT=y" >> $config_host_mak -fi if test "$pipe2" = "yes" ; then echo "CONFIG_PIPE2=y" >> $config_host_mak fi diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h index 900bdcb45a..629c8c648b 100644 --- a/include/sysemu/os-posix.h +++ b/include/sysemu/os-posix.h @@ -51,17 +51,6 @@ int os_mlock(void); typedef struct timeval qemu_timeval; #define qemu_gettimeofday(tp) gettimeofday(tp, NULL) -#ifndef CONFIG_UTIMENSAT -#ifndef UTIME_NOW -# define UTIME_NOW ((1l << 30) - 1l) -#endif -#ifndef UTIME_OMIT -# define UTIME_OMIT ((1l << 30) - 2l) -#endif -#endif -typedef struct timespec qemu_timespec; -int qemu_utimens(const char *path, const qemu_timespec *times); - bool is_daemonized(void); /** diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 4d9189e9ef..7e28c161b2 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -207,53 +207,6 @@ int qemu_pipe(int pipefd[2]) return ret; } -int qemu_utimens(const char *path, const struct timespec *times) -{ - struct timeval tv[2], tv_now; - struct stat st; - int i; -#ifdef CONFIG_UTIMENSAT - int ret; - - ret = utimensat(AT_FDCWD, path, times, AT_SYMLINK_NOFOLLOW); - if (ret != -1 || errno != ENOSYS) { - return ret; - } -#endif - /* Fallback: use utimes() instead of utimensat() */ - - /* happy if special cases */ - if (times[0].tv_nsec == UTIME_OMIT && times[1].tv_nsec == UTIME_OMIT) { - return 0; - } - if (times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec == UTIME_NOW) { - return utimes(path, NULL); - } - - /* prepare for hard cases */ - if (times[0].tv_nsec == UTIME_NOW || times[1].tv_nsec == UTIME_NOW) { - gettimeofday(&tv_now, NULL); - } - if (times[0].tv_nsec == UTIME_OMIT || times[1].tv_nsec == UTIME_OMIT) { - stat(path, &st); - } - - for (i = 0; i < 2; i++) { - if (times[i].tv_nsec == UTIME_NOW) { - tv[i].tv_sec = tv_now.tv_sec; - tv[i].tv_usec = tv_now.tv_usec; - } else if (times[i].tv_nsec == UTIME_OMIT) { - tv[i].tv_sec = (i == 0) ? st.st_atime : st.st_mtime; - tv[i].tv_usec = 0; - } else { - tv[i].tv_sec = times[i].tv_sec; - tv[i].tv_usec = times[i].tv_nsec / 1000; - } - } - - return utimes(path, &tv[0]); -} - char * qemu_get_local_state_pathname(const char *relative_pathname) { From 4fa62005d0a85df9bb14ea9f53109d7c5f66eeb9 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:14 +0200 Subject: [PATCH 08/11] 9pfs: check return value of v9fs_co_name_to_path() These v9fs_co_name_to_path() call sites have always been around. I guess no care was taken to check the return value because the name_to_path operation could never fail at the time. This is no longer true: the handle and synth backends can already fail this operation, and so will the local backend soon. Signed-off-by: Greg Kurz Reviewed-by: Eric Blake --- hw/9pfs/9p.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index a25d31e62f..96d2683348 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -2571,7 +2571,10 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp, err = -EINVAL; goto out; } - v9fs_co_name_to_path(pdu, &dirfidp->path, name->data, &new_path); + err = v9fs_co_name_to_path(pdu, &dirfidp->path, name->data, &new_path); + if (err < 0) { + goto out; + } } else { old_name = fidp->path.data; end = strrchr(old_name, '/'); @@ -2583,8 +2586,11 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp, new_name = g_malloc0(end - old_name + name->size + 1); strncat(new_name, old_name, end - old_name); strncat(new_name + (end - old_name), name->data, name->size); - v9fs_co_name_to_path(pdu, NULL, new_name, &new_path); + err = v9fs_co_name_to_path(pdu, NULL, new_name, &new_path); g_free(new_name); + if (err < 0) { + goto out; + } } err = v9fs_co_rename(pdu, &fidp->path, &new_path); if (err < 0) { @@ -2664,20 +2670,26 @@ out_nofid: v9fs_string_free(&name); } -static void coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, - V9fsString *old_name, - V9fsPath *newdir, - V9fsString *new_name) +static int coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, + V9fsString *old_name, + V9fsPath *newdir, + V9fsString *new_name) { V9fsFidState *tfidp; V9fsPath oldpath, newpath; V9fsState *s = pdu->s; - + int err; v9fs_path_init(&oldpath); v9fs_path_init(&newpath); - v9fs_co_name_to_path(pdu, olddir, old_name->data, &oldpath); - v9fs_co_name_to_path(pdu, newdir, new_name->data, &newpath); + err = v9fs_co_name_to_path(pdu, olddir, old_name->data, &oldpath); + if (err < 0) { + goto out; + } + err = v9fs_co_name_to_path(pdu, newdir, new_name->data, &newpath); + if (err < 0) { + goto out; + } /* * Fixup fid's pointing to the old name to @@ -2689,8 +2701,10 @@ static void coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data)); } } +out: v9fs_path_free(&oldpath); v9fs_path_free(&newpath); + return err; } static int coroutine_fn v9fs_complete_renameat(V9fsPDU *pdu, int32_t olddirfid, @@ -2724,8 +2738,8 @@ static int coroutine_fn v9fs_complete_renameat(V9fsPDU *pdu, int32_t olddirfid, } if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) { /* Only for path based fid we need to do the below fixup */ - v9fs_fix_fid_paths(pdu, &olddirfidp->path, old_name, - &newdirfidp->path, new_name); + err = v9fs_fix_fid_paths(pdu, &olddirfidp->path, old_name, + &newdirfidp->path, new_name); } out: if (olddirfidp) { From f57f5878578af19f72344439154234c6d6ba8ccc Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:14 +0200 Subject: [PATCH 09/11] 9pfs: local: resolve special directories in paths When using the mapped-file security mode, the creds of a path /foo/bar are stored in the /foo/.virtfs_metadata/bar file. This is okay for all paths unless they end with '.' or '..', because we cannot create the corresponding file in the metadata directory. This patch ensures that '.' and '..' are resolved in all paths. The core code only passes path elements (no '/') to the backend, with the notable exception of the '/' path, which refers to the virtfs root. This patch preserves the current behavior of converting it to '.' so that it can be passed to "*at()" syscalls ('/' would mean the host root). Signed-off-by: Greg Kurz Reviewed-by: Eric Blake --- hw/9pfs/9p-local.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 226234d386..68e92652ed 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1134,14 +1134,32 @@ static int local_name_to_path(FsContext *ctx, V9fsPath *dir_path, } if (dir_path) { - v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); - } else if (strcmp(name, "/")) { - v9fs_path_sprintf(target, "%s", name); + if (!strcmp(name, ".")) { + /* "." relative to "foo/bar" is "foo/bar" */ + v9fs_path_copy(target, dir_path); + } else if (!strcmp(name, "..")) { + if (!strcmp(dir_path->data, ".")) { + /* ".." relative to the root is "." */ + v9fs_path_sprintf(target, "."); + } else { + char *tmp = g_path_get_dirname(dir_path->data); + /* Symbolic links are resolved by the client. We can assume + * that ".." relative to "foo/bar" is equivalent to "foo" + */ + v9fs_path_sprintf(target, "%s", tmp); + g_free(tmp); + } + } else { + assert(!strchr(name, '/')); + v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); + } + } else if (!strcmp(name, "/") || !strcmp(name, ".") || + !strcmp(name, "..")) { + /* This is the root fid */ + v9fs_path_sprintf(target, "."); } else { - /* We want the path of the export root to be relative, otherwise - * "*at()" syscalls would treat it as "/" in the host. - */ - v9fs_path_sprintf(target, "%s", "."); + assert(!strchr(name, '/')); + v9fs_path_sprintf(target, "./%s", name); } return 0; } From 3dbcf27334b6c41e74a476b55d76f60df1c4007b Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:14 +0200 Subject: [PATCH 10/11] 9pfs: local: simplify file opening The logic to open a path currently sits between local_open_nofollow() and the relative_openat_nofollow() helper, which has no other user. For the sake of clarity, this patch moves all the code of the helper into its unique caller. While here we also: - drop the code to skip leading "/" because the backend isn't supposed to pass anything but relative paths without consecutive slashes. The assert() is kept because we really don't want a buggy backend to pass an absolute path to openat(). - use strchrnul() to get a simpler code. This is ok since virtfs is for linux+glibc hosts only. - don't dup() the initial directory and add an assert() to ensure we don't return the global mountfd to the caller. BTW, this would mean that the caller passed an empty path, which isn't supposed to happen either. Signed-off-by: Greg Kurz Reviewed-by: Eric Blake [groug: fixed typos in changelog] --- hw/9pfs/9p-local.c | 32 ++++++++++++++++++++++++++++---- hw/9pfs/9p-util.c | 43 ------------------------------------------- hw/9pfs/9p-util.h | 2 -- 3 files changed, 28 insertions(+), 49 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 68e92652ed..ddc5038cff 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -53,13 +53,37 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, mode_t mode) { LocalData *data = fs_ctx->private; + int fd = data->mountfd; - /* All paths are relative to the path data->mountfd points to */ - while (*path == '/') { - path++; + while (*path && fd != -1) { + const char *c; + int next_fd; + char *head; + + /* Only relative paths without consecutive slashes */ + assert(*path != '/'); + + head = g_strdup(path); + c = strchrnul(path, '/'); + if (*c) { + /* Intermediate path element */ + head[c - path] = 0; + path = c + 1; + next_fd = openat_dir(fd, head); + } else { + /* Rightmost path element */ + next_fd = openat_file(fd, head, flags, mode); + path = c; + } + g_free(head); + if (fd != data->mountfd) { + close_preserve_errno(fd); + } + fd = next_fd; } - return relative_openat_nofollow(data->mountfd, path, flags, mode); + assert(fd != data->mountfd); + return fd; } int local_opendir_nofollow(FsContext *fs_ctx, const char *path) diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c index fdb4d57376..f709c27a1f 100644 --- a/hw/9pfs/9p-util.c +++ b/hw/9pfs/9p-util.c @@ -14,49 +14,6 @@ #include "qemu/xattr.h" #include "9p-util.h" -int relative_openat_nofollow(int dirfd, const char *path, int flags, - mode_t mode) -{ - int fd; - - fd = dup(dirfd); - if (fd == -1) { - return -1; - } - - while (*path) { - const char *c; - int next_fd; - char *head; - - /* Only relative paths without consecutive slashes */ - assert(path[0] != '/'); - - head = g_strdup(path); - c = strchr(path, '/'); - if (c) { - head[c - path] = 0; - next_fd = openat_dir(fd, head); - } else { - next_fd = openat_file(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; - } - path = c + 1; - } - - return fd; -} - ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name, void *value, size_t size) { diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index 517027c520..91299a24b8 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -50,8 +50,6 @@ static inline int openat_file(int dirfd, const char *name, int flags, return fd; } -int relative_openat_nofollow(int dirfd, const char *path, int flags, - mode_t mode); ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name, void *value, size_t size); int fsetxattrat_nofollow(int dirfd, const char *path, const char *name, From 81ffbf5ab1458e357a761f1272105a55829b351e Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 25 May 2017 10:30:14 +0200 Subject: [PATCH 11/11] 9pfs: local: metadata file for the VirtFS root When using the mapped-file security, credentials are stored in a metadata directory located in the parent directory. This is okay for all paths with the notable exception of the root path, since we don't want and probably can't create a metadata directory above the virtfs directory on the host. This patch introduces a dedicated metadata file, sitting in the virtfs root for this purpose. It relies on the fact that the "." name necessarily refers to the virtfs root. As for the metadata directory, we don't want the client to see this file. The current code only cares for readdir() but there are many other places to fix actually. The filtering logic is hence put in a separate function. Before: # ls -ld drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . # chown root.root . chown: changing ownership of '.': Is a directory # ls -ld drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . After: # ls -ld drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . # chown root.root . # ls -ld drwxr-xr-x. 3 root root 4096 May 5 12:50 . and from the host: ls -al .virtfs_metadata_root -rwx------. 1 greg greg 26 May 5 12:50 .virtfs_metadata_root $ cat .virtfs_metadata_root virtfs.uid=0 virtfs.gid=0 Reported-by: Leo Gaspard Signed-off-by: Greg Kurz Reviewed-by: Eric Blake Tested-by: Leo Gaspard [groug: work around a patchew false positive in local_set_mapped_file_attrat()] --- hw/9pfs/9p-local.c | 86 +++++++++++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 27 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index ddc5038cff..1e78b7c9e9 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -107,6 +107,7 @@ static void unlinkat_preserve_errno(int dirfd, const char *path, int flags) } #define VIRTFS_META_DIR ".virtfs_metadata" +#define VIRTFS_META_ROOT_FILE VIRTFS_META_DIR "_root" static FILE *local_fopenat(int dirfd, const char *name, const char *mode) { @@ -143,13 +144,17 @@ static void local_mapped_file_attr(int dirfd, const char *name, char buf[ATTR_MAX]; int map_dirfd; - map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); - if (map_dirfd == -1) { - return; - } + if (strcmp(name, ".")) { + map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); + if (map_dirfd == -1) { + return; + } - fp = local_fopenat(map_dirfd, name, "r"); - close_preserve_errno(map_dirfd); + fp = local_fopenat(map_dirfd, name, "r"); + close_preserve_errno(map_dirfd); + } else { + fp = local_fopenat(dirfd, VIRTFS_META_ROOT_FILE, "r"); + } if (!fp) { return; } @@ -227,26 +232,38 @@ static int local_set_mapped_file_attrat(int dirfd, const char *name, int ret; char buf[ATTR_MAX]; int uid = -1, gid = -1, mode = -1, rdev = -1; - int map_dirfd; + int map_dirfd = -1, map_fd; + bool is_root = !strcmp(name, "."); - ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700); - if (ret < 0 && errno != EEXIST) { - return -1; - } - - map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); - if (map_dirfd == -1) { - return -1; - } - - fp = local_fopenat(map_dirfd, name, "r"); - if (!fp) { - if (errno == ENOENT) { - goto update_map_file; - } else { - close_preserve_errno(map_dirfd); + if (is_root) { + fp = local_fopenat(dirfd, VIRTFS_META_ROOT_FILE, "r"); + if (!fp) { + if (errno == ENOENT) { + goto update_map_file; + } else { + return -1; + } + } + } else { + ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700); + if (ret < 0 && errno != EEXIST) { return -1; } + + map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); + if (map_dirfd == -1) { + return -1; + } + + fp = local_fopenat(map_dirfd, name, "r"); + if (!fp) { + if (errno == ENOENT) { + goto update_map_file; + } else { + close_preserve_errno(map_dirfd); + return -1; + } + } } memset(buf, 0, ATTR_MAX); while (fgets(buf, ATTR_MAX, fp)) { @@ -264,12 +281,26 @@ static int local_set_mapped_file_attrat(int dirfd, const char *name, fclose(fp); update_map_file: - fp = local_fopenat(map_dirfd, name, "w"); - close_preserve_errno(map_dirfd); + if (is_root) { + fp = local_fopenat(dirfd, VIRTFS_META_ROOT_FILE, "w"); + } else { + fp = local_fopenat(map_dirfd, name, "w"); + /* We can't go this far with map_dirfd not being a valid file descriptor + * but some versions of gcc aren't smart enough to see it. + */ + if (map_dirfd != -1) { + close_preserve_errno(map_dirfd); + } + } if (!fp) { return -1; } + map_fd = fileno(fp); + assert(map_fd != -1); + ret = fchmod(map_fd, 0600); + assert(ret == 0); + if (credp->fc_uid != -1) { uid = credp->fc_uid; } @@ -478,7 +509,8 @@ static off_t local_telldir(FsContext *ctx, V9fsFidOpenState *fs) static bool local_is_mapped_file_metadata(FsContext *fs_ctx, const char *name) { - return !strcmp(name, VIRTFS_META_DIR); + return + !strcmp(name, VIRTFS_META_DIR) || !strcmp(name, VIRTFS_META_ROOT_FILE); } static struct dirent *local_readdir(FsContext *ctx, V9fsFidOpenState *fs) @@ -495,7 +527,7 @@ again: entry->d_type = DT_UNKNOWN; } else if (ctx->export_flags & V9FS_SM_MAPPED_FILE) { if (local_is_mapped_file_metadata(ctx, entry->d_name)) { - /* skip the meta data directory */ + /* skip the meta data */ goto again; } entry->d_type = DT_UNKNOWN;