thr3ads.net - Virtualization - [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device [Aug 2016]

If this information is useful, please help other people find it:
Share via:

Daniel P. Berrange

2016-Jul-28 13:22 UTC

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

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

Namhyung Kim

2016-Jul-30 08:57 UTC

head link

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

On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange
wrote:> 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 at redhat.com>
> > Cc: Radim Kr??m???? <rkrcmar at redhat.com>
> > Cc: "Michael S. Tsirkin" <mst at redhat.com>
> > Cc: Anthony Liguori <aliguori at amazon.com>
> > Cc: Anton Vorontsov <anton at enomsg.org>
> > Cc: Colin Cross <ccross at android.com>
> > Cc: Kees Cook <keescook at chromium.org>
> > Cc: Tony Luck <tony.luck at intel.com>
> > Cc: Steven Rostedt <rostedt at goodmis.org>
> > Cc: Ingo Molnar <mingo at kernel.org>
> > Cc: Minchan Kim <minchan at kernel.org>
> > Cc: kvm at vger.kernel.org
> > Cc: qemu-devel at nongnu.org
> > Cc: virtualization at lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim <namhyung at kernel.org>
> > ---
[SNIP]> > +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.
OK.
> 
> > +}
> > +
> > +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
Will change to use g_strdup_printf() as you said.
> 
> > +    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.
Will add a bound check then.
> 
> > +    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.
Do you mean that it'd be better to check a list of known filenames and
fail if not?
> 
> > +
> > +    /* 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.
Right.  The plan is to add size checking and to split if it exceeds
some limit.  And we can keep some number of recent files only.

Thanks,
Namhyung

> 
> > +    }
> > +
> > +    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
:|

Daniel P. Berrange

2016-Aug-01 09:24 UTC

head link

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

On Sat, Jul 30, 2016 at 05:57:02PM +0900, Namhyung Kim
wrote:> On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote:
> > > +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-");
> > > +    }
[snip]
> > > +    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.
> 
> Do you mean that it'd be better to check a list of known filenames and
> fail if not?
No, I mean that you have several different VIRTIO_PSTORE_TYPE_nnn and
use a different file for each constant. When reading this directory
though you're not looking for the file corresponding to any given
VIRTIO_PSTORE_TYPE_nnn - you're simply reading whichever file is found
first. So you might have just read a TYPE_CONSOLE or have read a
TYPE_DMESG - it surely doesn't make sense to randonly read either
TYPE_CONSOLE or TYPE_DMESG based on whatever order readdir() lists
the files.


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 :|

Possibly Parallel Threads

Search for more seemingly similar threads

Virtualization - Aug 2016 - [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

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

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

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

Possibly Parallel Threads