| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160718100353.GA15163@stefanha-x1.localdomain>
Date: Mon, 18 Jul 2016 11:03:53 +0100
From: Stefan Hajnoczi <stefanha@...il.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Namhyung Kim <namhyung@...il.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Radim Krčmář <rkrcmar@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>,
Anthony Liguori <aliguori@...zon.com>,
Anton Vorontsov <anton@...msg.org>,
Colin Cross <ccross@...roid.com>,
Kees Cook <keescook@...omium.org>,
Tony Luck <tony.luck@...el.com>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...nel.org>,
Minchan Kim <minchan@...nel.org>, kvm@...r.kernel.org,
qemu-devel@...gnu.org, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH 2/3] qemu: Implement virtio-pstore device
On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung@...il.com>
>
> Add virtio pstore device to allow kernel log files saved on the host.
> It will save the log files on the directory given by pstore device
> option.
>
> $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
>
> (guest) # echo c > /proc/sysrq-trigger
>
> $ ls dir-xx
> dmesg-0.enc.z dmesg-1.enc.z
>
> The log files are usually compressed using zlib. Users can see the log
> messages directly on the host or on the guest (using pstore filesystem).
The implementation is synchronous (i.e. can pause guest code execution),
does not handle write errors, and does not limit the amount of data the
guest can write. This is sufficient for ad-hoc debugging and usage with
trusted guests.
If you want this to be available in environments where the guest isn't
trusted then there must be a limit on how much the guest can write or
some kind of log rotation.
>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Radim Krčmář <rkrcmar@...hat.com>
> Cc: "Michael S. Tsirkin" <mst@...hat.com>
> Cc: Anthony Liguori <aliguori@...zon.com>
> Cc: Anton Vorontsov <anton@...msg.org>
> Cc: Colin Cross <ccross@...roid.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Tony Luck <tony.luck@...el.com>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Minchan Kim <minchan@...nel.org>
> Cc: kvm@...r.kernel.org
> Cc: qemu-devel@...gnu.org
> Cc: virtualization@...ts.linux-foundation.org
> Signed-off-by: Namhyung Kim <namhyung@...il.com>
> ---
> hw/virtio/Makefile.objs | 2 +-
> hw/virtio/virtio-pci.c | 50 ++++
> hw/virtio/virtio-pci.h | 14 +
> hw/virtio/virtio-pstore.c | 328 +++++++++++++++++++++
> include/hw/pci/pci.h | 1 +
> include/hw/virtio/virtio-pstore.h | 30 ++
> include/standard-headers/linux/virtio_ids.h | 1 +
> .../linux/{virtio_ids.h => virtio_pstore.h} | 48 +--
> qdev-monitor.c | 1 +
> 9 files changed, 455 insertions(+), 20 deletions(-)
> create mode 100644 hw/virtio/virtio-pstore.c
> create mode 100644 include/hw/virtio/virtio-pstore.h
> copy include/standard-headers/linux/{virtio_ids.h => virtio_pstore.h} (63%)
>
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 3e2b175..aae7082 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
> common-obj-y += virtio-mmio.o
>
> obj-y += virtio.o virtio-balloon.o
> -obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> +obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 2b34b43..8281b80 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2416,6 +2416,55 @@ static const TypeInfo virtio_host_pci_info = {
> };
> #endif
>
> +/* virtio-pstore-pci */
> +
> +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> + VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
> + DeviceState *vdev = DEVICE(&vps->vdev);
> + Error *err = NULL;
> +
> + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
> + object_property_set_bool(OBJECT(vdev), true, "realized", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> +}
> +
> +static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
> +
> + k->realize = virtio_pstore_pci_realize;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE;
> + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> + pcidev_k->class_id = PCI_CLASS_OTHERS;
> +}
> +
> +static void virtio_pstore_pci_instance_init(Object *obj)
> +{
> + VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj);
> +
> + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> + TYPE_VIRTIO_PSTORE);
> + object_property_add_alias(obj, "directory", OBJECT(&dev->vdev),
> + "directory", &error_abort);
> +}
> +
> +static const TypeInfo virtio_pstore_pci_info = {
> + .name = TYPE_VIRTIO_PSTORE_PCI,
> + .parent = TYPE_VIRTIO_PCI,
> + .instance_size = sizeof(VirtIOPstorePCI),
> + .instance_init = virtio_pstore_pci_instance_init,
> + .class_init = virtio_pstore_pci_class_init,
> +};
> +
> /* virtio-pci-bus */
>
> static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> @@ -2485,6 +2534,7 @@ static void virtio_pci_register_types(void)
> #ifdef CONFIG_VHOST_SCSI
> type_register_static(&vhost_scsi_pci_info);
> #endif
> + type_register_static(&virtio_pstore_pci_info);
> }
>
> type_init(virtio_pci_register_types)
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index e4548c2..b4c039f 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -31,6 +31,7 @@
> #ifdef CONFIG_VHOST_SCSI
> #include "hw/virtio/vhost-scsi.h"
> #endif
> +#include "hw/virtio/virtio-pstore.h"
>
> typedef struct VirtIOPCIProxy VirtIOPCIProxy;
> typedef struct VirtIOBlkPCI VirtIOBlkPCI;
> @@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
> typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
> typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
> typedef struct VirtIOGPUPCI VirtIOGPUPCI;
> +typedef struct VirtIOPstorePCI VirtIOPstorePCI;
>
> /* virtio-pci-bus */
>
> @@ -311,6 +313,18 @@ struct VirtIOGPUPCI {
> VirtIOGPU vdev;
> };
>
> +/*
> + * virtio-pstore-pci: This extends VirtioPCIProxy.
> + */
> +#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci"
> +#define VIRTIO_PSTORE_PCI(obj) \
> + OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI)
> +
> +struct VirtIOPstorePCI {
> + VirtIOPCIProxy parent_obj;
> + VirtIOPstore vdev;
> +};
> +
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> #define VIRTIO_PCI_ABI_VERSION 0
>
> diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> new file mode 100644
> index 0000000..98cee7f
> --- /dev/null
> +++ b/hw/virtio/virtio-pstore.c
> @@ -0,0 +1,328 @@
> +/*
> + * Virtio Pstore Device
> + *
> + * Copyright (C) 2016 LG Electronics
> + *
> + * Authors:
> + * Namhyung Kim <namhyung@...il.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <stdio.h>
> +
> +#include "qemu/osdep.h"
> +#include "qemu/iov.h"
> +#include "qemu-common.h"
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "qapi/visitor.h"
> +#include "qapi-event.h"
> +#include "trace.h"
> +
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> +#include "hw/virtio/virtio-pstore.h"
> +
> +
> +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + const char *basename;
> +
> + switch (hdr->type) {
Missing le16_to_cpu()?
> + case VIRTIO_PSTORE_TYPE_DMESG:
> + basename = "dmesg";
> + break;
> + default:
> + basename = "unknown";
> + break;
> + }
> +
> + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename,
> + (unsigned long long) hdr->id,
Missing le64_to_cpu()?
> + hdr->flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");
Missing le32_to_cpu()?
> +}
> +
> +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> + char *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + size_t len = strlen(name);
> +
> + hdr->flags = 0;
> + if (!strncmp(name + len - 6, ".enc.z", 6)) {
Please use g_str_has_suffix(name, ".enc.z") to avoid accessing before
the beginning of the string if the filename is shorter than 6
characters.
> + hdr->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> + }
> +
> + snprintf(buf, sz, "%s/%s", s->directory, name);
> +
> + if (!strncmp(name, "dmesg-", 6)) {
g_str_has_prefix(name, "dmesg-")
> + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_DMESG);
> + name += 6;
> + } else if (!strncmp(name, "unknown-", 8)) {
g_str_has_prefix(name, "unknown-")
> + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
> + name += 8;
> + }
> +
> + qemu_strtoull(name, NULL, 0, &hdr->id);
> +}
> +
> +static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
> +{
> + s->dir = opendir(s->directory);
> + if (s->dir == NULL) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, void *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + char path[PATH_MAX];
> + FILE *fp;
> + ssize_t len;
> + struct stat stbuf;
> + struct dirent *dent;
> +
> + if (s->dir == NULL) {
> + return -1;
> + }
> +
> + dent = readdir(s->dir);
> + while (dent) {
> + if (dent->d_name[0] != '.') {
> + break;
> + }
> + dent = readdir(s->dir);
> + }
> +
> + if (dent == NULL) {
> + return 0;
> + }
> +
> + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), hdr);
> + if (stat(path, &stbuf) < 0) {
> + return -1;
> + }
Please use fstat(fileno(fp), &stbuf) after opening the file instead.
The race condition doesn't matter in this case but the race-free code is
just as simple so it's one less thing someone reading the code has to
worry about.
> +
> + fp = fopen(path, "r");
> + if (fp == NULL) {
> + error_report("cannot open %s (%p %p)", path, s, s->directory);
> + return -1;
> + }
> +
> + len = fread(buf, 1, sz, fp);
> + if (len < 0 && errno == EAGAIN) {
> + len = 0;
> + }
> +
> + hdr->id = cpu_to_le64(hdr->id);
> + hdr->flags = cpu_to_le32(hdr->flags);
> + hdr->time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec);
> + hdr->time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
> +
> + fclose(fp);
> + return len;
> +}
> +
> +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, void *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + char path[PATH_MAX];
> + FILE *fp;
> +
> + virtio_pstore_to_filename(s, path, sizeof(path), hdr);
> +
> + fp = fopen(path, "w");
> + if (fp == NULL) {
> + error_report("cannot open %s (%p %p)", path, s, s->directory);
> + return -1;
> + }
> + fwrite(buf, 1, sz, fp);
> + fclose(fp);
> +
> + return sz;
> +}
> +
> +static ssize_t virtio_pstore_do_close(VirtIOPstore *s)
> +{
> + if (s->dir == NULL) {
> + return 0;
> + }
> +
> + closedir(s->dir);
> + s->dir = NULL;
> +
> + return 0;
> +}
> +
> +static ssize_t virtio_pstore_do_erase(VirtIOPstore *s,
> + struct virtio_pstore_hdr *hdr)
> +{
> + char path[PATH_MAX];
> +
> + virtio_pstore_to_filename(s, path, sizeof(path), hdr);
> +
> + return unlink(path);
> +}
> +
> +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> +{
> + VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> + VirtQueueElement *elem;
> + struct virtio_pstore_hdr *hdr;
> + ssize_t len;
> +
> + for (;;) {
> + elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> + if (!elem) {
> + return;
> + }
> +
> + hdr = elem->out_sg[0].iov_base;
> + if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
> + error_report("invalid header size: %u",
> + (unsigned)elem->out_sg[0].iov_len);
> + exit(1);
> + }
Please use iov_to_buf() instead of directly accessing out_sg[]. Virtio
devices are not supposed to assume a particular iovec layout. In other
words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.
You must also copy in data (similar to Linux syscall implementations) to
prevent the guest from modifying data while the command is processed.
Such race conditions could lead to security bugs.
> +
> + switch (hdr->cmd) {
> + case VIRTIO_PSTORE_CMD_OPEN:
> + len = virtio_pstore_do_open(s);
> + break;
> + case VIRTIO_PSTORE_CMD_READ:
> + len = virtio_pstore_do_read(s, elem->in_sg[0].iov_base,
> + elem->in_sg[0].iov_len, hdr);
Same issue with iovec layout for in_sg[] here. The guest driver must be
able to submit any in_sg[] iovec array and the device cannot assume
in_sg[0] is the only iovec to fill.
> + break;
> + case VIRTIO_PSTORE_CMD_WRITE:
> + len = virtio_pstore_do_write(s, elem->out_sg[1].iov_base,
> + elem->out_sg[1].iov_len, hdr);
> + break;
> + case VIRTIO_PSTORE_CMD_CLOSE:
> + len = virtio_pstore_do_close(s);
> + break;
> + case VIRTIO_PSTORE_CMD_ERASE:
> + len = virtio_pstore_do_erase(s, hdr);
> + break;
> + default:
> + len = -1;
> + break;
> + }
> +
> + if (len < 0) {
> + return;
> + }
> +
> + virtqueue_push(vq, elem, len);
> +
> + virtio_notify(vdev, vq);
> + g_free(elem);
> + }
> +}
> +
> +static void virtio_pstore_device_realize(DeviceState *dev, Error **errp)
> +{
> + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> + VirtIOPstore *s = VIRTIO_PSTORE(dev);
> +
> + virtio_init(vdev, "virtio-pstore", VIRTIO_ID_PSTORE, 0);
> +
> + s->vq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io);
> +}
> +
> +static void virtio_pstore_device_unrealize(DeviceState *dev, Error **errp)
> +{
> + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +
> + virtio_cleanup(vdev);
> +}
> +
> +static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
> +{
> + return f;
> +}
> +
> +static void pstore_get_directory(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> +
> + visit_type_str(v, name, &s->directory, errp);
> +}
> +
> +static void pstore_set_directory(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + Error *local_err = NULL;
> + char *value;
> +
> + visit_type_str(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + g_free(s->directory);
> + s->directory = strdup(value);
Please use g_strdup() since this is paired with g_free().
Or even simpler would be s->directory = value and do not g_free(value)
below.
> +
> + g_free(value);
> +}
> +
> +static void pstore_release_directory(Object *obj, const char *name,
> + void *opaque)
> +{
> + VirtIOPstore *s = opaque;
> +
> + g_free(s->directory);
> + s->directory = NULL;
> +}
> +
> +static Property virtio_pstore_properties[] = {
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void virtio_pstore_instance_init(Object *obj)
> +{
> + VirtIOPstore *s = VIRTIO_PSTORE(obj);
> +
> + object_property_add(obj, "directory", "str",
> + pstore_get_directory, pstore_set_directory,
> + pstore_release_directory, s, NULL);
> +}
> +
> +static void virtio_pstore_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +
> + dc->props = virtio_pstore_properties;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> + vdc->realize = virtio_pstore_device_realize;
> + vdc->unrealize = virtio_pstore_device_unrealize;
> + vdc->get_features = get_features;
> +}
> +
> +static const TypeInfo virtio_pstore_info = {
> + .name = TYPE_VIRTIO_PSTORE,
> + .parent = TYPE_VIRTIO_DEVICE,
> + .instance_size = sizeof(VirtIOPstore),
> + .instance_init = virtio_pstore_instance_init,
> + .class_init = virtio_pstore_class_init,
> +};
> +
> +static void virtio_register_types(void)
> +{
> + type_register_static(&virtio_pstore_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 9ed1624..5689c6f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -79,6 +79,7 @@
> #define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004
> #define PCI_DEVICE_ID_VIRTIO_RNG 0x1005
> #define PCI_DEVICE_ID_VIRTIO_9P 0x1009
> +#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a
>
> #define PCI_VENDOR_ID_REDHAT 0x1b36
> #define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001
> diff --git a/include/hw/virtio/virtio-pstore.h b/include/hw/virtio/virtio-pstore.h
> new file mode 100644
> index 0000000..74cd1f6
> --- /dev/null
> +++ b/include/hw/virtio/virtio-pstore.h
> @@ -0,0 +1,30 @@
> +/*
> + * Virtio Pstore Support
> + *
> + * Authors:
> + * Namhyung Kim <namhyung@...il.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _QEMU_VIRTIO_PSTORE_H
> +#define _QEMU_VIRTIO_PSTORE_H
> +
> +#include "standard-headers/linux/virtio_pstore.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/pci/pci.h"
> +
> +#define TYPE_VIRTIO_PSTORE "virtio-pstore-device"
> +#define VIRTIO_PSTORE(obj) \
> + OBJECT_CHECK(VirtIOPstore, (obj), TYPE_VIRTIO_PSTORE)
> +
> +typedef struct VirtIOPstore {
> + VirtIODevice parent_obj;
> + VirtQueue *vq;
> + char *directory;
> + DIR *dir;
> +} VirtIOPstore;
> +
> +#endif
> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> index 77925f5..cba6322 100644
> --- a/include/standard-headers/linux/virtio_ids.h
> +++ b/include/standard-headers/linux/virtio_ids.h
> @@ -41,5 +41,6 @@
> #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> #define VIRTIO_ID_GPU 16 /* virtio GPU */
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
19 has already been reserved. 22 is the next free ID (vsock, crypto,
and sdm are currently under review and already use 19, 20, and 21).
Please send a VIRTIO draft specification to
virtio-dev@...ts.oasis-open.org. You can find information on the VIRTIO
standards process here:
https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio
Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)
Powered by blists - more mailing lists