On Wed, Jul 20, 2016 at 09:21:08AM +0100, Stefan Hajnoczi wrote:

On Wed, Jul 20, 2016 at 12:48:39AM +0900, Namhyung Kim wrote:

quoted

Hello,

On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:

quoted

On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:

quoted

+static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOPstore *s = VIRTIO_PSTORE(vdev);
+    VirtQueueElement *elem;
+    struct virtio_pstore_hdr *hdr;
+    ssize_t len;
+
+    for (;;) {
+        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+        if (!elem) {
+            return;
+        }
+
+        hdr = elem->out_sg[0].iov_base;
+        if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
+            error_report("invalid header size: %u",
+                         (unsigned)elem->out_sg[0].iov_len);
+            exit(1);
+        }

Please use iov_to_buf() instead of directly accessing out_sg[].  Virtio
devices are not supposed to assume a particular iovec layout.  In other
words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.

You must also copy in data (similar to Linux syscall implementations) to
prevent the guest from modifying data while the command is processed.
Such race conditions could lead to security bugs.

By accessing elem->out_sg[0].iov_base directly, I abused it as an
in-and-out buffer.  But it seems not allowed by the virtio spec, do I
have to use separate buffers for request and response?

Yes, a virtqueue element has (host read-only) out buffers followed by
(host write-only) in buffers.

Thanks for clarification.  I'll split them.

Thanks,
Namhyung