[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] vhost: secure vhost shared log files using argv
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH] vhost: secure vhost shared log files using argv paremeter |
|
Date: |
Sun, 30 Oct 2016 21:26:51 +0200 |
On Sat, Oct 22, 2016 at 07:00:41AM +0000, Rafael David Tinoco wrote:
> Commit 31190ed7 added a migration blocker in vhost_dev_init() to
> check if memfd would succeed. It is better if this blocker first
> checks if vhost backend requires shared log. This will avoid a
> situation where a blocker is added inappropriately (e.g. shared
> log allocation fails when vhost backend doesn't support it).
Sounds like a bugfix but I'm not sure. Can this part be split
out in a patch by itself?
> Commit: 35f9b6e added a fallback mechanism for systems not supporting
> memfd_create syscall (started being supported since 3.17).
>
> Backporting memfd_create might not be accepted for distros relying
> on older kernels. Nowadays there is no way for security driver
> to discover memfd filename to be created: <tmpdir>/memfd-XXXXXX.
>
> Also, because vhost log file descriptors can be passed to other
> processes, after discussion, we thought it is best to back mmap by
> using files that can be placed into a specific directory: this commit
> creates "vhostlog" argv parameter for such purpose. This will allow
> security drivers to operate on those files appropriately.
>
> Argv examples:
>
> -netdev tap,id=net0,vhost=on
> -netdev tap,id=net0,vhost=on,vhostlog=/tmp/guest.log
> -netdev tap,id=net0,vhost=on,vhostlog=/tmp
>
> For vhost backends supporting shared logs, if vhostlog is non-existent,
> or a directory, random files are going to be created in the specified
> directory (or, for non-existent, in tmpdir). If vhostlog is specified,
> the filepath is always used when allocating vhost log files.
When vhostlog is not specified, can we just use memfd as we did?
> Signed-off-by: Rafael David Tinoco <address@hidden>
> ---
> hw/net/vhost_net.c | 4 +-
> hw/scsi/vhost-scsi.c | 2 +-
> hw/virtio/vhost-vsock.c | 2 +-
> hw/virtio/vhost.c | 41 +++++++------
> include/hw/virtio/vhost.h | 4 +-
> include/net/vhost_net.h | 1 +
> include/qemu/mmap-file.h | 10 +++
> net/tap.c | 6 ++
> qapi-schema.json | 3 +
> qemu-options.hx | 3 +-
> util/Makefile.objs | 1 +
> util/mmap-file.c | 153
> ++++++++++++++++++++++++++++++++++++++++++++++
> 12 files changed, 207 insertions(+), 23 deletions(-)
> create mode 100644 include/qemu/mmap-file.h
> create mode 100644 util/mmap-file.c
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index f2d49ad..d650c92 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -171,8 +171,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> net->dev.vq_index = net->nc->queue_index * net->dev.nvqs;
> }
>
> - r = vhost_dev_init(&net->dev, options->opaque,
> - options->backend_type, options->busyloop_timeout);
> + r = vhost_dev_init(&net->dev, options->opaque, options->backend_type,
> + options->busyloop_timeout, options->vhostlog);
> if (r < 0) {
> goto fail;
> }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 5b26946..5dc3d30 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -248,7 +248,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error
> **errp)
> s->dev.backend_features = 0;
>
> ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
> - VHOST_BACKEND_TYPE_KERNEL, 0);
> + VHOST_BACKEND_TYPE_KERNEL, 0, NULL);
> if (ret < 0) {
> error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
> strerror(-ret));
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index b481562..6cf6081 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -342,7 +342,7 @@ static void vhost_vsock_device_realize(DeviceState *dev,
> Error **errp)
> vsock->vhost_dev.nvqs = ARRAY_SIZE(vsock->vhost_vqs);
> vsock->vhost_dev.vqs = vsock->vhost_vqs;
> ret = vhost_dev_init(&vsock->vhost_dev, (void *)(uintptr_t)vhostfd,
> - VHOST_BACKEND_TYPE_KERNEL, 0);
> + VHOST_BACKEND_TYPE_KERNEL, 0, NULL);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "vhost-vsock: vhost_dev_init failed");
> goto err_virtio;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index bd051ab..d874ebb 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -20,7 +20,7 @@
> #include "qemu/atomic.h"
> #include "qemu/range.h"
> #include "qemu/error-report.h"
> -#include "qemu/memfd.h"
> +#include "qemu/mmap-file.h"
> #include <linux/vhost.h>
> #include "exec/address-spaces.h"
> #include "hw/virtio/virtio-bus.h"
> @@ -326,7 +326,7 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
> return log_size;
> }
>
> -static struct vhost_log *vhost_log_alloc(uint64_t size, bool share)
> +static struct vhost_log *vhost_log_alloc(char *path, uint64_t size, bool
> share)
> {
> struct vhost_log *log;
> uint64_t logsize = size * sizeof(*(log->log));
> @@ -334,9 +334,7 @@ static struct vhost_log *vhost_log_alloc(uint64_t size,
> bool share)
>
> log = g_new0(struct vhost_log, 1);
> if (share) {
> - log->log = qemu_memfd_alloc("vhost-log", logsize,
> - F_SEAL_GROW | F_SEAL_SHRINK |
> F_SEAL_SEAL,
> - &fd);
> + log->log = qemu_mmap_alloc(path, logsize, &fd);
> memset(log->log, 0, logsize);
> } else {
> log->log = g_malloc0(logsize);
> @@ -349,12 +347,12 @@ static struct vhost_log *vhost_log_alloc(uint64_t size,
> bool share)
> return log;
> }
>
> -static struct vhost_log *vhost_log_get(uint64_t size, bool share)
> +static struct vhost_log *vhost_log_get(char *path, uint64_t size, bool share)
> {
> struct vhost_log *log = share ? vhost_log_shm : vhost_log;
>
> if (!log || log->size != size) {
> - log = vhost_log_alloc(size, share);
> + log = vhost_log_alloc(path, size, share);
> if (share) {
> vhost_log_shm = log;
> } else {
> @@ -388,8 +386,7 @@ static void vhost_log_put(struct vhost_dev *dev, bool
> sync)
> g_free(log->log);
> vhost_log = NULL;
> } else if (vhost_log_shm == log) {
> - qemu_memfd_free(log->log, log->size * sizeof(*(log->log)),
> - log->fd);
> + qemu_mmap_free(log->log, log->size * sizeof(*(log->log)),
> log->fd);
> vhost_log_shm = NULL;
> }
>
> @@ -405,9 +402,12 @@ static bool vhost_dev_log_is_shared(struct vhost_dev
> *dev)
>
> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> {
> - struct vhost_log *log = vhost_log_get(size,
> vhost_dev_log_is_shared(dev));
> - uint64_t log_base = (uintptr_t)log->log;
> int r;
> + struct vhost_log *log;
> + uint64_t log_base;
> +
> + log = vhost_log_get(dev->log_filename, size,
> vhost_dev_log_is_shared(dev));
> + log_base = (uintptr_t)log->log;
>
> /* inform backend of log switching, this must be done before
> releasing the current log, to ensure no logging is lost */
> @@ -1049,7 +1049,8 @@ static void vhost_virtqueue_cleanup(struct
> vhost_virtqueue *vq)
> }
>
> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> - VhostBackendType backend_type, uint32_t busyloop_timeout)
> + VhostBackendType backend_type,
> + uint32_t busyloop_timeout, char *vhostlog)
> {
> uint64_t features;
> int i, r, n_initialized_vqs = 0;
> @@ -1118,11 +1119,18 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> *opaque,
> .priority = 10
> };
>
> + hdev->log = NULL;
> + hdev->log_size = 0;
> + hdev->log_enabled = false;
> + hdev->log_filename = vhostlog ? g_strdup(vhostlog) : NULL;
> + g_free(vhostlog);
> +
> if (hdev->migration_blocker == NULL) {
> if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) {
> error_setg(&hdev->migration_blocker,
> "Migration disabled: vhost lacks VHOST_F_LOG_ALL
> feature.");
> - } else if (!qemu_memfd_check()) {
> + } else if (vhost_dev_log_is_shared(hdev) &&
> + !qemu_mmap_check(hdev->log_filename)) {
> error_setg(&hdev->migration_blocker,
> "Migration disabled: failed to allocate shared
> memory");
> }
> @@ -1135,9 +1143,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
> hdev->n_mem_sections = 0;
> hdev->mem_sections = NULL;
> - hdev->log = NULL;
> - hdev->log_size = 0;
> - hdev->log_enabled = false;
> hdev->started = false;
> hdev->memory_changed = false;
> memory_listener_register(&hdev->memory_listener, &address_space_memory);
> @@ -1175,6 +1180,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> if (hdev->vhost_ops) {
> hdev->vhost_ops->vhost_backend_cleanup(hdev);
> }
> + g_free(hdev->log_filename);
> assert(!hdev->log);
>
> memset(hdev, 0, sizeof(struct vhost_dev));
> @@ -1335,7 +1341,8 @@ int vhost_dev_start(struct vhost_dev *hdev,
> VirtIODevice *vdev)
> uint64_t log_base;
>
> hdev->log_size = vhost_get_log_size(hdev);
> - hdev->log = vhost_log_get(hdev->log_size,
> + hdev->log = vhost_log_get(hdev->log_filename,
> + hdev->log_size,
> vhost_dev_log_is_shared(hdev));
> log_base = (uintptr_t)hdev->log->log;
> r = hdev->vhost_ops->vhost_set_log_base(hdev,
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index e433089..1ea4f3a 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -52,6 +52,7 @@ struct vhost_dev {
> uint64_t max_queues;
> bool started;
> bool log_enabled;
> + char *log_filename;
> uint64_t log_size;
> Error *migration_blocker;
> bool memory_changed;
> @@ -65,7 +66,8 @@ struct vhost_dev {
>
> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> VhostBackendType backend_type,
> - uint32_t busyloop_timeout);
> + uint32_t busyloop_timeout,
> + char *vhostlog);
> void vhost_dev_cleanup(struct vhost_dev *hdev);
> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 5a08eff..94161b2 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -12,6 +12,7 @@ typedef struct VhostNetOptions {
> NetClientState *net_backend;
> uint32_t busyloop_timeout;
> void *opaque;
> + char *vhostlog;
> } VhostNetOptions;
>
> uint64_t vhost_net_get_max_queues(VHostNetState *net);
> diff --git a/include/qemu/mmap-file.h b/include/qemu/mmap-file.h
> new file mode 100644
> index 0000000..427612a
> --- /dev/null
> +++ b/include/qemu/mmap-file.h
> @@ -0,0 +1,10 @@
> +#ifndef QEMU_MMAP_FILE_H
> +#define QEMU_MMAP_FILE_H
> +
> +#include "qemu-common.h"
> +
> +void *qemu_mmap_alloc(const char *path, size_t size, int *fd);
> +void qemu_mmap_free(void *ptr, size_t size, int fd);
> +bool qemu_mmap_check(const char *path);
> +
> +#endif
> diff --git a/net/tap.c b/net/tap.c
> index b6896a7..7b242cd 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -699,6 +699,12 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
> }
> options.opaque = (void *)(uintptr_t)vhostfd;
>
> + if (tap->has_vhostlog) {
> + options.vhostlog = g_strdup(tap->vhostlog);
> + } else {
> + options.vhostlog = NULL;
> + }
> +
> s->vhost_net = vhost_net_init(&options);
> if (!s->vhost_net) {
> error_setg(errp,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5a8ec38..72608bd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2640,6 +2640,8 @@
> #
> # @vhostforce: #optional vhost on for non-MSIX virtio guests
> #
> +# @vhostlog: #optional file or directory for vhost backend log
> +#
> # @queues: #optional number of queues to be created for multiqueue capable
> tap
> #
> # @poll-us: #optional maximum number of microseconds that could
> @@ -2662,6 +2664,7 @@
> '*vhostfd': 'str',
> '*vhostfds': 'str',
> '*vhostforce': 'bool',
> + '*vhostlog': 'str',
> '*queues': 'uint32',
> '*poll-us': 'uint32'} }
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b1fbdb0..5c09c09 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1599,7 +1599,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> #else
> "-netdev
> tap,id=str[,fd=h][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile]\n"
> "
> [,br=bridge][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n"
> - "
> [,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n]\n"
> + "
> [,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,vhostlog=file|dir][,queues=n]\n"
> " [,poll-us=n]\n"
> " configure a host TAP network backend with ID 'str'\n"
> " connected to a bridge (default="
> DEFAULT_BRIDGE_INTERFACE ")\n"
> @@ -1618,6 +1618,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> " use vhost=on to enable experimental in kernel
> accelerator\n"
> " (only has effect for virtio guests which use
> MSIX)\n"
> " use vhostforce=on to force vhost on for non-MSIX virtio
> guests\n"
> + " use 'vhostlog=file|dir' file or directory for vhost
> backend log\n"
> " use 'vhostfd=h' to connect to an already opened vhost
> net device\n"
> " use 'vhostfds=x:y:...:z to connect to multiple already
> opened vhost net devices\n"
> " use 'queues=n' to specify the number of queues to be
> created for multiqueue TAP\n"
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 36c7dcc..69bb27a 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -3,6 +3,7 @@ util-obj-y += bufferiszero.o
> util-obj-$(CONFIG_POSIX) += compatfd.o
> util-obj-$(CONFIG_POSIX) += event_notifier-posix.o
> util-obj-$(CONFIG_POSIX) += mmap-alloc.o
> +util-obj-$(CONFIG_POSIX) += mmap-file.o
> util-obj-$(CONFIG_POSIX) += oslib-posix.o
> util-obj-$(CONFIG_POSIX) += qemu-openpty.o
> util-obj-$(CONFIG_POSIX) += qemu-thread-posix.o
> diff --git a/util/mmap-file.c b/util/mmap-file.c
> new file mode 100644
> index 0000000..ce778cf
> --- /dev/null
> +++ b/util/mmap-file.c
> @@ -0,0 +1,153 @@
> +/*
> + * Support for file backed by mmaped host memory.
> + *
> + * Authors:
> + * Rafael David Tinoco <address@hidden>
> + *
> + * 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 "qemu/mmap-file.h"
> +
> +static char *qemu_mmap_rand_name(void)
> +{
> + char *name;
> + GRand *rsufix;
> + guint32 sufix;
> +
> + rsufix = g_rand_new();
> + sufix = g_rand_int(rsufix);
> + g_free(rsufix);
> + name = g_strdup_printf("mmap-%u", sufix);
> +
> + return name;
> +}
> +
> +static inline void qemu_mmap_rand_name_free(char *str)
> +{
> + g_free(str);
> +}
> +
> +static bool qemu_mmap_is(const char *path, mode_t what)
> +{
> + struct stat s;
> +
> + memset(&s, 0, sizeof(struct stat));
> + if (stat(path, &s)) {
> + perror("stat");
> + goto negative;
> + }
> +
> + if ((s.st_mode & S_IFMT) == what) {
> + return true;
> + }
> +
> +negative:
> + return false;
> +}
> +
> +static inline bool qemu_mmap_is_file(const char *path)
> +{
> + return qemu_mmap_is(path, S_IFREG);
> +}
> +
> +static inline bool qemu_mmap_is_dir(const char *path)
> +{
> + return qemu_mmap_is(path, S_IFDIR);
> +}
> +
> +static void *qemu_mmap_alloc_file(const char *filepath, size_t size, int *fd)
> +{
> + void *ptr;
> + int mfd = -1;
> +
> + *fd = -1;
> +
> + mfd = open(filepath, O_CREAT | O_EXCL | O_RDWR, S_IRUSR | S_IWUSR);
> + if (mfd == -1) {
> + perror("open");
> + return NULL;
> + }
> +
> + unlink(filepath);
> +
> + if (ftruncate(mfd, size) == -1) {
> + perror("ftruncate");
> + close(mfd);
> + return NULL;
> + }
> +
> + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> + if (ptr == MAP_FAILED) {
> + perror("mmap");
> + close(mfd);
> + return NULL;
> + }
> +
> + *fd = mfd;
> + return ptr;
> +}
> +
> +static void *qemu_mmap_alloc_dir(const char *dirpath, size_t size, int *fd)
> +{
> + void *ptr;
> + char *file, *rand, *tmp, *dir2use = NULL;
> +
> + if (dirpath && !qemu_mmap_is_dir(dirpath)) {
> + return NULL;
> + }
> +
> + tmp = (char *) g_get_tmp_dir();
> + dir2use = dirpath ? (char *) dirpath : tmp;
> + rand = qemu_mmap_rand_name();
> + file = g_strdup_printf("%s/%s", dir2use, rand);
> + ptr = qemu_mmap_alloc_file(file, size, fd);
> + g_free(tmp);
> + qemu_mmap_rand_name_free(rand);
> +
> + return ptr;
> +}
> +
> +/*
> + * "path" can be:
> + *
> + * filename = full path for the file to back mmap
> + * dir path = full dir path where to create random file for mmap
> + * null = will use <tmpdir> to create random file for mmap
> + */
> +void *qemu_mmap_alloc(const char *path, size_t size, int *fd)
> +{
> + if (!path || qemu_mmap_is_dir(path)) {
> + return qemu_mmap_alloc_dir(path, size, fd);
> + }
> +
> + return qemu_mmap_alloc_file(path, size, fd);
> +}
> +
> +void qemu_mmap_free(void *ptr, size_t size, int fd)
> +{
> + if (ptr) {
> + munmap(ptr, size);
> + }
> +
> + if (fd != -1) {
> + close(fd);
> + }
> +}
> +
> +bool qemu_mmap_check(const char *path)
> +{
> + void *ptr;
> + int fd = -1;
> + bool r = true;
> +
> + ptr = qemu_mmap_alloc(path, 4096, &fd);
> + if (!ptr) {
> + r = false;
> + }
> + qemu_mmap_free(ptr, 4096, fd);
> +
> + return r == true ? true : false;
> +}
> --
> 2.9.3