From: Stefan Hajnoczi <stefanha@redhat.com> To: Wei Wang <wei.w.wang@intel.com> Cc: virtio-dev@lists.oasis-open.org, qemu-devel@nongnu.org, mst@redhat.com, marcandre.lureau@redhat.com, jasowang@redhat.com, pbonzini@redhat.com, jan.kiszka@siemens.com, avi.cohen@huawei.com, zhiyong.yang@intel.com Subject: Re: [Qemu-devel] [PATCH v3 4/7] vhost-pci-slave: add vhost-pci slave implementation Date: Tue, 5 Dec 2017 15:56:01 +0000 [thread overview] Message-ID: <20171205155601.GG31150@stefanha-x1.localdomain> (raw) In-Reply-To: <1512444796-30615-5-git-send-email-wei.w.wang@intel.com> [-- Attachment #1: Type: text/plain, Size: 2862 bytes --] On Tue, Dec 05, 2017 at 11:33:13AM +0800, Wei Wang wrote: > +static int vp_slave_write(CharBackend *chr_be, VhostUserMsg *msg) > +{ > + int size; > + > + if (!msg) { > + return 0; > + } > + > + /* The payload size has been assigned, plus the header size here */ > + size = msg->size + VHOST_USER_HDR_SIZE; > + msg->flags &= ~VHOST_USER_VERSION_MASK; > + msg->flags |= VHOST_USER_VERSION; > + > + return qemu_chr_fe_write_all(chr_be, (const uint8_t *)msg, size) > + == size ? 0 : -1; > +} qemu_chr_fe_write_all() is a blocking operation. If the socket fd cannot accept more data then this thread will block! This is a reliability problem. If the vhost-user master process hangs then the vhost-pci vhost-user slave will also hang :(. Please implement vhost-pci so that it does not hang. A guest with multiple vhost-pci devices should work even if one or more of them cannot communicate with the vhost-pci master. This is necessary for preventing denial-of-service on a software-defined network switch, for example. > +static void vp_slave_set_vring_num(VhostPCINet *vpnet, VhostUserMsg *msg) > +{ > + struct vhost_vring_state *state = &msg->payload.state; > + > + vpnet->metadata->vq[state->index].vring_num = state->num; The vhost-pci code cannot trust vhost-user input. This function allows the vhost-user master to perform out-of-bounds memory stores by setting state->index outside the vq[] array. All input must be validated! The security model is: 1. Guest must be able to corrupt QEMU memory or execute arbitrary code. 2. vhost-user master guest must not be able to corrupt vhost-user slave guest memory or execute arbitrary code. 3. vhost-user master must not be able to corrupt vhost-user memory or execute arbitrary code in vhost-user slave. 4. vhost-user slave must not be able to corrupt vhost-user memory or execute arbitrary code in vhost-user master. The only thing that is allowed is vhost-user slave QEMU and guest may write to vhost-user master guest memory. > +void vp_slave_read(void *opaque, const uint8_t *buf, int size) > +{ > + int ret, fd_num, fds[VHOST_MEMORY_MAX_NREGIONS]; > + VhostUserMsg msg; > + uint8_t *p = (uint8_t *) &msg; > + VhostPCINet *vpnet = (VhostPCINet *)opaque; > + CharBackend *chr_be = &vpnet->chr_be; > + > + if (size != VHOST_USER_HDR_SIZE) { > + error_report("%s: wrong message size received %d", __func__, size); > + return; > + } > + > + memcpy(p, buf, VHOST_USER_HDR_SIZE); > + > + if (msg.size) { > + p += VHOST_USER_HDR_SIZE; > + size = qemu_chr_fe_read_all(chr_be, p, msg.size); This is a blocking operation. See my comment about qemu_chr_fe_write_all(). This is also a buffer overflow since msg.size is not validated. All input from the vhost-user master needs to be validated. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Stefan Hajnoczi <stefanha@redhat.com> To: Wei Wang <wei.w.wang@intel.com> Cc: virtio-dev@lists.oasis-open.org, qemu-devel@nongnu.org, mst@redhat.com, marcandre.lureau@redhat.com, jasowang@redhat.com, pbonzini@redhat.com, jan.kiszka@siemens.com, avi.cohen@huawei.com, zhiyong.yang@intel.com Subject: [virtio-dev] Re: [PATCH v3 4/7] vhost-pci-slave: add vhost-pci slave implementation Date: Tue, 5 Dec 2017 15:56:01 +0000 [thread overview] Message-ID: <20171205155601.GG31150@stefanha-x1.localdomain> (raw) In-Reply-To: <1512444796-30615-5-git-send-email-wei.w.wang@intel.com> [-- Attachment #1: Type: text/plain, Size: 2862 bytes --] On Tue, Dec 05, 2017 at 11:33:13AM +0800, Wei Wang wrote: > +static int vp_slave_write(CharBackend *chr_be, VhostUserMsg *msg) > +{ > + int size; > + > + if (!msg) { > + return 0; > + } > + > + /* The payload size has been assigned, plus the header size here */ > + size = msg->size + VHOST_USER_HDR_SIZE; > + msg->flags &= ~VHOST_USER_VERSION_MASK; > + msg->flags |= VHOST_USER_VERSION; > + > + return qemu_chr_fe_write_all(chr_be, (const uint8_t *)msg, size) > + == size ? 0 : -1; > +} qemu_chr_fe_write_all() is a blocking operation. If the socket fd cannot accept more data then this thread will block! This is a reliability problem. If the vhost-user master process hangs then the vhost-pci vhost-user slave will also hang :(. Please implement vhost-pci so that it does not hang. A guest with multiple vhost-pci devices should work even if one or more of them cannot communicate with the vhost-pci master. This is necessary for preventing denial-of-service on a software-defined network switch, for example. > +static void vp_slave_set_vring_num(VhostPCINet *vpnet, VhostUserMsg *msg) > +{ > + struct vhost_vring_state *state = &msg->payload.state; > + > + vpnet->metadata->vq[state->index].vring_num = state->num; The vhost-pci code cannot trust vhost-user input. This function allows the vhost-user master to perform out-of-bounds memory stores by setting state->index outside the vq[] array. All input must be validated! The security model is: 1. Guest must be able to corrupt QEMU memory or execute arbitrary code. 2. vhost-user master guest must not be able to corrupt vhost-user slave guest memory or execute arbitrary code. 3. vhost-user master must not be able to corrupt vhost-user memory or execute arbitrary code in vhost-user slave. 4. vhost-user slave must not be able to corrupt vhost-user memory or execute arbitrary code in vhost-user master. The only thing that is allowed is vhost-user slave QEMU and guest may write to vhost-user master guest memory. > +void vp_slave_read(void *opaque, const uint8_t *buf, int size) > +{ > + int ret, fd_num, fds[VHOST_MEMORY_MAX_NREGIONS]; > + VhostUserMsg msg; > + uint8_t *p = (uint8_t *) &msg; > + VhostPCINet *vpnet = (VhostPCINet *)opaque; > + CharBackend *chr_be = &vpnet->chr_be; > + > + if (size != VHOST_USER_HDR_SIZE) { > + error_report("%s: wrong message size received %d", __func__, size); > + return; > + } > + > + memcpy(p, buf, VHOST_USER_HDR_SIZE); > + > + if (msg.size) { > + p += VHOST_USER_HDR_SIZE; > + size = qemu_chr_fe_read_all(chr_be, p, msg.size); This is a blocking operation. See my comment about qemu_chr_fe_write_all(). This is also a buffer overflow since msg.size is not validated. All input from the vhost-user master needs to be validated. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --]
next prev parent reply other threads:[~2017-12-05 15:56 UTC|newest] Thread overview: 170+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-12-05 3:33 [Qemu-devel] [PATCH v3 0/7] Vhost-pci for inter-VM communication Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 3:33 ` [Qemu-devel] [PATCH v3 1/7] vhost-user: share the vhost-user protocol related structures Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 3:33 ` [Qemu-devel] [PATCH v3 2/7] vhost-pci-net: add vhost-pci-net Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 14:59 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-05 14:59 ` Stefan Hajnoczi 2017-12-05 15:17 ` [Qemu-devel] " Michael S. Tsirkin 2017-12-05 15:17 ` Michael S. Tsirkin 2017-12-05 15:55 ` [Qemu-devel] " Michael S. Tsirkin 2017-12-05 15:55 ` Michael S. Tsirkin 2017-12-05 16:41 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-05 16:41 ` Stefan Hajnoczi 2017-12-05 16:53 ` [Qemu-devel] " Michael S. Tsirkin 2017-12-05 16:53 ` Michael S. Tsirkin 2017-12-05 17:00 ` [Qemu-devel] " Cornelia Huck 2017-12-05 17:00 ` [virtio-dev] " Cornelia Huck 2017-12-05 18:06 ` Michael S. Tsirkin 2017-12-05 18:06 ` [virtio-dev] " Michael S. Tsirkin 2017-12-06 10:17 ` Wei Wang 2017-12-06 10:17 ` Wei Wang 2017-12-06 12:01 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-06 12:01 ` Stefan Hajnoczi 2017-12-05 3:33 ` [Qemu-devel] [PATCH v3 3/7] virtio/virtio-pci.c: add vhost-pci-net-pci Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 3:33 ` [Qemu-devel] [PATCH v3 4/7] vhost-pci-slave: add vhost-pci slave implementation Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 15:56 ` Stefan Hajnoczi [this message] 2017-12-05 15:56 ` [virtio-dev] " Stefan Hajnoczi 2017-12-14 17:30 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-14 17:30 ` [virtio-dev] " Stefan Hajnoczi 2017-12-14 17:48 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-14 17:48 ` [virtio-dev] " Stefan Hajnoczi 2017-12-05 3:33 ` [Qemu-devel] [PATCH v3 5/7] vhost-user: VHOST_USER_SET_VHOST_PCI msg Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 16:00 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-05 16:00 ` [virtio-dev] " Stefan Hajnoczi 2017-12-06 10:32 ` [Qemu-devel] " Wei Wang 2017-12-06 10:32 ` Wei Wang 2017-12-15 12:40 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-15 12:40 ` Stefan Hajnoczi 2017-12-05 3:33 ` [Qemu-devel] [PATCH v3 6/7] vhost-pci-slave: handle VHOST_USER_SET_VHOST_PCI Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 3:33 ` [Qemu-devel] [PATCH v3 7/7] virtio/vhost.c: vhost-pci needs remote gpa Wei Wang 2017-12-05 3:33 ` [virtio-dev] " Wei Wang 2017-12-05 16:05 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-05 16:05 ` [virtio-dev] " Stefan Hajnoczi 2017-12-06 10:46 ` [Qemu-devel] " Wei Wang 2017-12-06 10:46 ` Wei Wang 2017-12-05 4:13 ` [Qemu-devel] [PATCH v3 0/7] Vhost-pci for inter-VM communication no-reply 2017-12-05 7:01 ` [Qemu-devel] [virtio-dev] " Jason Wang 2017-12-05 7:01 ` Jason Wang 2017-12-05 7:15 ` [Qemu-devel] " Wei Wang 2017-12-05 7:15 ` Wei Wang 2017-12-05 7:19 ` [Qemu-devel] " Jason Wang 2017-12-05 7:19 ` Jason Wang 2017-12-05 8:49 ` [Qemu-devel] " Avi Cohen (A) 2017-12-05 8:49 ` Avi Cohen (A) 2017-12-05 10:36 ` [Qemu-devel] " Wei Wang 2017-12-05 10:36 ` Wei Wang 2017-12-05 14:30 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-05 14:30 ` Stefan Hajnoczi 2017-12-05 15:20 ` [Qemu-devel] " Michael S. Tsirkin 2017-12-05 15:20 ` [virtio-dev] " Michael S. Tsirkin 2017-12-05 16:06 ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi 2017-12-05 16:06 ` Stefan Hajnoczi 2017-12-06 13:49 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-06 13:49 ` Stefan Hajnoczi 2017-12-06 16:09 ` [Qemu-devel] " Wang, Wei W 2017-12-06 16:09 ` Wang, Wei W 2017-12-06 16:27 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-07 3:57 ` Wei Wang 2017-12-07 3:57 ` [virtio-dev] " Wei Wang 2017-12-07 5:11 ` Michael S. Tsirkin 2017-12-07 5:11 ` [virtio-dev] " Michael S. Tsirkin 2017-12-07 5:34 ` Wei Wang 2017-12-07 5:34 ` [virtio-dev] " Wei Wang 2017-12-07 6:31 ` Stefan Hajnoczi 2017-12-07 7:54 ` Avi Cohen (A) 2017-12-07 7:54 ` [virtio-dev] " Avi Cohen (A) 2017-12-07 8:04 ` Stefan Hajnoczi 2017-12-07 8:31 ` Jason Wang 2017-12-07 8:31 ` [virtio-dev] " Jason Wang 2017-12-07 10:24 ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi 2017-12-07 10:24 ` [virtio-dev] Re: [Qemu-devel] " Stefan Hajnoczi 2017-12-07 13:33 ` Michael S. Tsirkin 2017-12-07 13:33 ` [virtio-dev] " Michael S. Tsirkin 2017-12-07 9:02 ` Wei Wang 2017-12-07 9:02 ` [virtio-dev] " Wei Wang 2017-12-07 13:08 ` Stefan Hajnoczi 2017-12-07 14:02 ` Michael S. Tsirkin 2017-12-07 14:02 ` [virtio-dev] " Michael S. Tsirkin 2017-12-07 16:29 ` Stefan Hajnoczi 2017-12-07 16:47 ` Michael S. Tsirkin 2017-12-07 16:47 ` [virtio-dev] " Michael S. Tsirkin 2017-12-07 17:29 ` Stefan Hajnoczi 2017-12-07 17:38 ` Michael S. Tsirkin 2017-12-07 17:38 ` [virtio-dev] " Michael S. Tsirkin 2017-12-07 18:28 ` Stefan Hajnoczi 2017-12-07 23:54 ` Michael S. Tsirkin 2017-12-07 23:54 ` [virtio-dev] " Michael S. Tsirkin 2017-12-08 6:08 ` Stefan Hajnoczi 2017-12-08 14:27 ` Michael S. Tsirkin 2017-12-08 14:27 ` [virtio-dev] " Michael S. Tsirkin 2017-12-08 16:15 ` Stefan Hajnoczi 2017-12-09 16:08 ` Wang, Wei W 2017-12-09 16:08 ` [virtio-dev] " Wang, Wei W 2017-12-08 6:43 ` Wei Wang 2017-12-08 6:43 ` [virtio-dev] " Wei Wang 2017-12-08 8:33 ` Stefan Hajnoczi 2017-12-09 16:23 ` Wang, Wei W 2017-12-09 16:23 ` [virtio-dev] " Wang, Wei W 2017-12-11 11:11 ` Stefan Hajnoczi 2017-12-11 11:11 ` [virtio-dev] " Stefan Hajnoczi 2017-12-11 13:53 ` Wang, Wei W 2017-12-11 13:53 ` [virtio-dev] " Wang, Wei W 2017-12-12 10:14 ` Stefan Hajnoczi 2017-12-12 10:14 ` [virtio-dev] " Stefan Hajnoczi 2017-12-13 8:11 ` Wei Wang 2017-12-13 8:11 ` [virtio-dev] " Wei Wang 2017-12-13 12:35 ` Stefan Hajnoczi 2017-12-13 15:01 ` Michael S. Tsirkin 2017-12-13 15:01 ` [virtio-dev] " Michael S. Tsirkin 2017-12-13 20:08 ` Stefan Hajnoczi 2017-12-13 20:59 ` Michael S. Tsirkin 2017-12-13 20:59 ` [virtio-dev] " Michael S. Tsirkin 2017-12-14 15:06 ` Stefan Hajnoczi 2017-12-14 15:06 ` [virtio-dev] " Stefan Hajnoczi 2017-12-15 10:33 ` Wei Wang 2017-12-15 10:33 ` [virtio-dev] " Wei Wang 2017-12-15 12:37 ` Stefan Hajnoczi 2017-12-15 12:37 ` [virtio-dev] " Stefan Hajnoczi 2017-12-13 21:50 ` Maxime Coquelin 2017-12-13 21:50 ` [virtio-dev] " Maxime Coquelin 2017-12-14 15:46 ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi 2017-12-14 15:46 ` [virtio-dev] Re: [Qemu-devel] " Stefan Hajnoczi 2017-12-14 16:27 ` [Qemu-devel] [virtio-dev] " Michael S. Tsirkin 2017-12-14 16:27 ` [virtio-dev] Re: [Qemu-devel] " Michael S. Tsirkin 2017-12-14 16:39 ` [Qemu-devel] [virtio-dev] " Maxime Coquelin 2017-12-14 16:39 ` [virtio-dev] Re: [Qemu-devel] " Maxime Coquelin 2017-12-14 16:40 ` [Qemu-devel] [virtio-dev] " Michael S. Tsirkin 2017-12-14 16:40 ` [virtio-dev] Re: [Qemu-devel] " Michael S. Tsirkin 2017-12-14 16:50 ` [Qemu-devel] [virtio-dev] " Maxime Coquelin 2017-12-14 16:50 ` [virtio-dev] Re: [Qemu-devel] " Maxime Coquelin 2017-12-14 18:11 ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi 2017-12-14 18:11 ` [virtio-dev] Re: [Qemu-devel] " Stefan Hajnoczi 2017-12-14 5:53 ` Wei Wang 2017-12-14 5:53 ` [virtio-dev] " Wei Wang 2017-12-14 17:32 ` Stefan Hajnoczi 2017-12-14 17:32 ` [virtio-dev] " Stefan Hajnoczi 2017-12-15 9:10 ` Wei Wang 2017-12-15 9:10 ` [virtio-dev] " Wei Wang 2017-12-15 12:26 ` Stefan Hajnoczi 2017-12-15 12:26 ` [virtio-dev] " Stefan Hajnoczi 2017-12-14 18:04 ` Stefan Hajnoczi 2017-12-14 18:04 ` [virtio-dev] " Stefan Hajnoczi 2017-12-15 10:33 ` [Qemu-devel] [virtio-dev] " Wei Wang 2017-12-15 10:33 ` [virtio-dev] Re: [Qemu-devel] " Wei Wang 2017-12-15 12:00 ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi 2017-12-15 12:00 ` [virtio-dev] Re: [Qemu-devel] " Stefan Hajnoczi 2017-12-06 16:13 ` Stefan Hajnoczi 2017-12-19 11:35 ` Stefan Hajnoczi 2017-12-19 11:35 ` Stefan Hajnoczi 2017-12-19 14:56 ` [Qemu-devel] " Michael S. Tsirkin 2017-12-19 14:56 ` Michael S. Tsirkin 2017-12-19 17:05 ` [Qemu-devel] " Stefan Hajnoczi 2017-12-20 4:06 ` Michael S. Tsirkin 2017-12-20 4:06 ` [virtio-dev] " Michael S. Tsirkin 2017-12-20 6:26 ` Stefan Hajnoczi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20171205155601.GG31150@stefanha-x1.localdomain \ --to=stefanha@redhat.com \ --cc=avi.cohen@huawei.com \ --cc=jan.kiszka@siemens.com \ --cc=jasowang@redhat.com \ --cc=marcandre.lureau@redhat.com \ --cc=mst@redhat.com \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=virtio-dev@lists.oasis-open.org \ --cc=wei.w.wang@intel.com \ --cc=zhiyong.yang@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.