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

[Namhyung Kim]

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 <address@hidden>
> > Cc: Radim Kr??m???? <address@hidden>
> > Cc: "Michael S. Tsirkin" <address@hidden>
> > Cc: Anthony Liguori <address@hidden>
> > Cc: Anton Vorontsov <address@hidden>
> > Cc: Colin Cross <address@hidden>
> > Cc: Kees Cook <address@hidden>
> > Cc: Tony Luck <address@hidden>
> > Cc: Steven Rostedt <address@hidden>
> > Cc: Ingo Molnar <address@hidden>
> > Cc: Minchan Kim <address@hidden>
> > Cc: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > Signed-off-by: Namhyung Kim <address@hidden>
> > ---

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