Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

From: Daniel P. Berrange
Date: Thu Jul 28 2016 - 09:22:57 EST

Next message: zyjzyj2000: "[PATCH 1/1] ixgbevf: replace integer number with bool value"
Previous message: LABBE Corentin: "Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac"
In reply to: Steven Rostedt: "Re: [PATCH 6/7] qemu: Implement virtio-pstore device"
Next in thread: Namhyung Kim: "Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> 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-1.enc.z dmesg-2.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 'directory' property is required for virtio-pstore device to work.
> It also adds 'bufsize' and 'console' (boolean) properties.
>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> Cc: Anthony Liguori <aliguori@xxxxxxxxxx>
> Cc: Anton Vorontsov <anton@xxxxxxxxxx>
> Cc: Colin Cross <ccross@xxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Tony Luck <tony.luck@xxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Minchan Kim <minchan@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: qemu-devel@xxxxxxxxxx
> Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> hw/virtio/Makefile.objs | 2 +-
> hw/virtio/virtio-pci.c | 54 +++
> hw/virtio/virtio-pci.h | 14 +
> hw/virtio/virtio-pstore.c | 477 +++++++++++++++++++++++++
> include/hw/pci/pci.h | 1 +
> include/hw/virtio/virtio-pstore.h | 34 ++
> include/standard-headers/linux/virtio_ids.h | 1 +
> include/standard-headers/linux/virtio_pstore.h | 80 +++++
> qdev-monitor.c | 1 +
> 9 files changed, 663 insertions(+), 1 deletion(-)
> create mode 100644 hw/virtio/virtio-pstore.c
> create mode 100644 include/hw/virtio/virtio-pstore.h
> create mode 100644 include/standard-headers/linux/virtio_pstore.h
>
> 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 f0677b7..d99a405 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2414,6 +2414,59 @@ 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);
> + object_property_add_alias(obj, "bufsize", OBJECT(&dev->vdev),
> + "bufsize", &error_abort);
> + object_property_add_alias(obj, "console", OBJECT(&dev->vdev),
> + "console", &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,
> @@ -2483,6 +2536,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..2ca7786
> --- /dev/null
> +++ b/hw/virtio/virtio-pstore.c
> @@ -0,0 +1,477 @@
> +/*
> + * Virtio Pstore Device
> + *
> + * Copyright (C) 2016 LG Electronics
> + *
> + * Authors:
> + * Namhyung Kim <namhyung@xxxxxxxxx>
> + *
> + * 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_req *req)
> +{
> + const char *basename;
> + unsigned long long id = 0;
> + unsigned int flags = le32_to_cpu(req->flags);
> +
> + switch (le16_to_cpu(req->type)) {
> + case VIRTIO_PSTORE_TYPE_DMESG:
> + basename = "dmesg";
> + id = s->id++;
> + break;
> + case VIRTIO_PSTORE_TYPE_CONSOLE:
> + basename = "console";
> + if (s->console_id) {
> + id = s->console_id;
> + } else {
> + id = s->console_id = s->id++;
> + }
> + break;
> + default:
> + basename = "unknown";
> + break;
> + }
> +
> + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id,
> + flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");

Please use g_strdup_printf() instead of splattering into a pre-allocated
buffer than may or may not be large enough.

> +}
> +
> +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> + char *buf, size_t sz,
> + struct virtio_pstore_fileinfo *info)
> +{
> + snprintf(buf, sz, "%s/%s", s->directory, name);
> +
> + if (g_str_has_prefix(name, "dmesg-")) {
> + info->type = VIRTIO_PSTORE_TYPE_DMESG;
> + name += strlen("dmesg-");
> + } else if (g_str_has_prefix(name, "console-")) {
> + info->type = VIRTIO_PSTORE_TYPE_CONSOLE;
> + name += strlen("console-");
> + } else if (g_str_has_prefix(name, "unknown-")) {
> + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> + name += strlen("unknown-");
> + }
> +
> + qemu_strtoull(name, NULL, 0, &info->id);
> +
> + info->flags = 0;
> + if (g_str_has_suffix(name, ".enc.z")) {
> + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> + }
> +}
> +
> +static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
> +{
> + s->dirp = opendir(s->directory);
> + if (s->dirp == NULL) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg,
> + unsigned int in_num,
> + struct virtio_pstore_res *res)
> +{
> + char path[PATH_MAX];

Don't declare PATH_MAX sized variables

> + int fd;
> + ssize_t len;
> + struct stat stbuf;
> + struct dirent *dent;
> + int sg_num = in_num;
> + struct iovec sg[sg_num];

'sg_num' is initialized from 'in_num' which comes from the
guest, and I'm not seeing anything which is bounds-checking
the 'in_num' value. So you've possibly got a security flaw here
I think, if the guest can cause QEMU to allocate arbitrary stack
memory & thus overflow by setting arbitrarily large in_num.

> + struct virtio_pstore_fileinfo info;
> + size_t offset = sizeof(*res) + sizeof(info);
> +
> + if (s->dirp == NULL) {
> + return -1;
> + }
> +
> + dent = readdir(s->dirp);
> + while (dent) {
> + if (dent->d_name[0] != '.') {
> + break;
> + }
> + dent = readdir(s->dirp);
> + }
> +
> + if (dent == NULL) {
> + return 0;
> + }

So this seems to just be picking the first filename reported by
readdir that isn't starting with '.'. Surely this can't the right
logic when your corresponding do_write method can pick several
different filenames, its potluck which do_read will give back.

> +
> + /* skip res and fileinfo */
> + sg_num = iov_copy(sg, sg_num, in_sg, in_num, offset,
> + iov_size(in_sg, in_num) - offset);
> +
> + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), &info);
> + fd = open(path, O_RDONLY);
> + if (fd < 0) {
> + error_report("cannot open %s", path);
> + return -1;
> + }
> +
> + if (fstat(fd, &stbuf) < 0) {
> + len = -1;
> + goto out;
> + }
> +
> + len = readv(fd, sg, sg_num);
> + if (len < 0) {
> + if (errno == EAGAIN) {
> + len = 0;
> + }
> + goto out;
> + }
> +
> + info.id = cpu_to_le64(info.id);
> + info.type = cpu_to_le16(info.type);
> + info.flags = cpu_to_le32(info.flags);
> + info.len = cpu_to_le32(len);
> + info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec);
> + info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
> +
> + iov_from_buf(in_sg, in_num, sizeof(*res), &info, sizeof(info));
> + len += sizeof(info);
> +
> + out:
> + close(fd);
> + return len;
> +}
> +
> +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg,
> + unsigned int out_num,
> + struct virtio_pstore_req *req)
> +{
> + char path[PATH_MAX];
> + int fd;
> + ssize_t len;
> + unsigned short type;
> + int flags = O_WRONLY | O_CREAT;
> +
> + /* we already consume the req */
> + iov_discard_front(&out_sg, &out_num, sizeof(*req));
> +
> + virtio_pstore_to_filename(s, path, sizeof(path), req);
> +
> + type = le16_to_cpu(req->type);
> +
> + if (type == VIRTIO_PSTORE_TYPE_DMESG) {
> + flags |= O_TRUNC;
> + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) {
> + flags |= O_APPEND;

Using O_APPEND will cause the file to grow without bound on the
host, which is highly undesirable, aka a security flaw.

> + }
> +
> + fd = open(path, flags, 0644);
> + if (fd < 0) {
> + error_report("cannot open %s", path);
> + return -1;
> + }
> + len = writev(fd, out_sg, out_num);
> + close(fd);
> +
> + return len;
> +}

Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Next message: zyjzyj2000: "[PATCH 1/1] ixgbevf: replace integer number with bool value"
Previous message: LABBE Corentin: "Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac"
In reply to: Steven Rostedt: "Re: [PATCH 6/7] qemu: Implement virtio-pstore device"
Next in thread: Namhyung Kim: "Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]