linux-kernel - Re: [PATCH 2/3] qemu: Implement virtio-pstore device

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160922122316.GB8221@stefanha-x1.localdomain>
Date:   Thu, 22 Sep 2016 13:23:16 +0100
From:   Stefan Hajnoczi <stefanha@...il.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     virtio-dev@...ts.oasis-open.org,
        Anton Vorontsov <anton@...msg.org>,
        "Daniel P . Berrange" <berrange@...hat.com>,
        Kees Cook <keescook@...omium.org>, kvm@...r.kernel.org,
        Radim Krčmář <rkrcmar@...hat.com>,
        qemu-devel@...gnu.org, Namhyung Kim <namhyung@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        virtualization@...ts.linux-foundation.org,
        Minchan Kim <minchan@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        Anthony Liguori <aliguori@...zon.com>,
        Colin Cross <ccross@...roid.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH 2/3] qemu: Implement virtio-pstore device

On Sun, Sep 04, 2016 at 11:38:59PM +0900, Namhyung Kim wrote:
> +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> +    VirtQueueElement *elem;
> +    struct virtio_pstore_req req;
> +    struct virtio_pstore_res res;
> +    ssize_t len = 0;
> +    int ret;
> +
> +    for (;;) {
> +        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +        if (!elem) {
> +            return;
> +        }
> +
> +        if (elem->out_num < 1 || elem->in_num < 1) {
> +            error_report("request or response buffer is missing");
> +            exit(1);

The new virtio_error() function might be available, depending on when
this patch series is merged.  virtio_error() should be used instead of
exit(1).  See "[PATCH v5 0/9] virtio: avoid exit() when device enters
invalid states" on qemu-devel.

> +        }
> +
> +        if (elem->out_num > 2 || elem->in_num > 3) {
> +            error_report("invalid number of input/output buffer");
> +            exit(1);
> +        }

The VIRTIO specification requires that flexible framing is supported.
The device cannot make assumptions about the scatter-gather list.  It
must support any layout (e.g. even multiple 1-byte iovecs making up the
buffer).

> +
> +        len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req));
> +        if (len != (ssize_t)sizeof(req)) {
> +            error_report("invalid request size: %ld", (long)len);
> +            exit(1);
> +        }
> +        res.cmd  = req.cmd;
> +        res.type = req.type;
> +
> +        switch (le16_to_cpu(req.cmd)) {
> +        case VIRTIO_PSTORE_CMD_OPEN:
> +            ret = virtio_pstore_do_open(s);
> +            break;
> +        case VIRTIO_PSTORE_CMD_CLOSE:
> +            ret = virtio_pstore_do_close(s);
> +            break;
> +        case VIRTIO_PSTORE_CMD_ERASE:
> +            ret = virtio_pstore_do_erase(s, &req);
> +            break;
> +        case VIRTIO_PSTORE_CMD_READ:
> +            ret = virtio_pstore_do_read(s, elem);
> +            if (ret == 1) {
> +                /* async channel io */
> +                continue;
> +            }
> +            break;
> +        case VIRTIO_PSTORE_CMD_WRITE:
> +            ret = virtio_pstore_do_write(s, elem, &req);
> +            if (ret == 1) {
> +                /* async channel io */
> +                continue;
> +            }
> +            break;
> +        default:
> +            ret = -1;
> +            break;
> +        }
> +
> +        res.ret = ret;

Missing cpu_to_le()?

> +static void pstore_set_bufsize(Object *obj, Visitor *v,
> +                               const char *name, void *opaque,
> +                               Error **errp)
> +{
> +    VirtIOPstore *s = opaque;
> +    Error *error = NULL;
> +    uint64_t value;
> +
> +    visit_type_size(v, name, &value, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    if (value < 4096) {
> +        error_setg(&error, "Warning: too small buffer size: %"PRIu64, value);

This is an error, not a warning.  Please remove "Warning:" so it's clear
to the user that this message caused QEMU to fail.

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)