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

["Daniel P. Berrange" <berrange@redhat.com>]

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@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  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@gmail.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_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 :|