Hello, On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:> On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote: > > +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? Thanks, Namhyung
On Wed, Jul 20, 2016 at 12:48:39AM +0900, Namhyung Kim wrote:> Hello, > > On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote: > > On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote: > > > +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. Stefan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20160720/ae6e6148/attachment.sig>
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: > > Hello, > > > > On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote: > > > On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote: > > > > +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