| Message ID | 147447703245.30952.11628276217402153393.stgit@bahia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 21 Sep 2016 18:57:12 +0200 Greg Kurz <groug@kaod.org> wrote: > A broken guest may send a request with only non-empty out buffers > or only non-empty in buffers, virtqueue_pop() will then return a > VirtQueueElement with out_num == 0 or in_num == 0 respectively. > > All 9P requests are expected to start with the following 7-byte header: > > uint32_t size_le; > uint8_t id; > uint16_t tag_le; > > If iov_to_buf() fails to return these 7 bytes, then something is wrong in > the guest. > > In both cases, it is wrong to crash QEMU, since the root cause lies in the > guest. Let's switch the device to the broken state instead. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > v2: - added out_free_pdu: label for errors or when virtqueue is empty > --- > hw/9pfs/virtio-9p-device.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
On Wed, Sep 21, 2016 at 06:57:12PM +0200, Greg Kurz wrote: > A broken guest may send a request with only non-empty out buffers > or only non-empty in buffers, virtqueue_pop() will then return a > VirtQueueElement with out_num == 0 or in_num == 0 respectively. > > All 9P requests are expected to start with the following 7-byte header: > > uint32_t size_le; > uint8_t id; > uint16_t tag_le; > > If iov_to_buf() fails to return these 7 bytes, then something is wrong in > the guest. > > In both cases, it is wrong to crash QEMU, since the root cause lies in the > guest. Let's switch the device to the broken state instead. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > v2: - added out_free_pdu: label for errors or when virtqueue is empty > --- > hw/9pfs/virtio-9p-device.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > index e7ea0e45f3dd..5f3a67cfc717 100644 > --- a/hw/9pfs/virtio-9p-device.c > +++ b/hw/9pfs/virtio-9p-device.c > @@ -52,17 +52,24 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) > > elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > if (!elem) { > - pdu_free(pdu); > - break; > + goto out_free_pdu; > } > > - BUG_ON(elem->out_num == 0 || elem->in_num == 0); > + if (elem->out_num == 0 || elem->in_num == 0) { > + virtio_error(vdev, > + "The guest sent a VirtFS request without headers"); > + goto out_free_pdu; > + } I'm not sure this check is even necessary since we check iov_to_buf(elem->out_sg[]) and pdu_marshall() avoids overflowing elem->in_sg[]. Either way, your change is correct: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Fri, 23 Sep 2016 13:54:17 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Wed, Sep 21, 2016 at 06:57:12PM +0200, Greg Kurz wrote: > > A broken guest may send a request with only non-empty out buffers > > or only non-empty in buffers, virtqueue_pop() will then return a > > VirtQueueElement with out_num == 0 or in_num == 0 respectively. > > > > All 9P requests are expected to start with the following 7-byte header: > > > > uint32_t size_le; > > uint8_t id; > > uint16_t tag_le; > > > > If iov_to_buf() fails to return these 7 bytes, then something is wrong in > > the guest. > > > > In both cases, it is wrong to crash QEMU, since the root cause lies in the > > guest. Let's switch the device to the broken state instead. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > v2: - added out_free_pdu: label for errors or when virtqueue is empty > > --- > > hw/9pfs/virtio-9p-device.c | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > > index e7ea0e45f3dd..5f3a67cfc717 100644 > > --- a/hw/9pfs/virtio-9p-device.c > > +++ b/hw/9pfs/virtio-9p-device.c > > @@ -52,17 +52,24 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) > > > > elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > > if (!elem) { > > - pdu_free(pdu); > > - break; > > + goto out_free_pdu; > > } > > > > - BUG_ON(elem->out_num == 0 || elem->in_num == 0); > > + if (elem->out_num == 0 || elem->in_num == 0) { > > + virtio_error(vdev, > > + "The guest sent a VirtFS request without headers"); > > + goto out_free_pdu; > > + } > > I'm not sure this check is even necessary since we check > iov_to_buf(elem->out_sg[]) and pdu_marshall() avoids overflowing > elem->in_sg[]. > I agree that the difference between absent and empty out headers is quite subtile indeed. It would probably be acceptable to rely on iov_to_buf(), the same way virtio-net relies on iov_size() I guess. The case of elem->in_sg[] seems different though: unless I missed something, v9fs_packunpack() will simply return -ENOBUFS. The error will be propagated to pdu_complete(), which does not even check for errors (see the comment). And then we'll push a broken element to the guest... I'd rather raise the flag here. > Either way, your change is correct: > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c index e7ea0e45f3dd..5f3a67cfc717 100644 --- a/hw/9pfs/virtio-9p-device.c +++ b/hw/9pfs/virtio-9p-device.c @@ -52,17 +52,24 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); if (!elem) { - pdu_free(pdu); - break; + goto out_free_pdu; } - BUG_ON(elem->out_num == 0 || elem->in_num == 0); + if (elem->out_num == 0 || elem->in_num == 0) { + virtio_error(vdev, + "The guest sent a VirtFS request without headers"); + goto out_free_pdu; + } QEMU_BUILD_BUG_ON(sizeof(out) != 7); v->elems[pdu->idx] = elem; len = iov_to_buf(elem->out_sg, elem->out_num, 0, &out, sizeof(out)); - BUG_ON(len != sizeof(out)); + if (len != sizeof(out)) { + virtio_error(vdev, "The guest sent a malformed VirtFS request: " + "header size is %zd, should be 7", len); + goto out_free_pdu; + } pdu->size = le32_to_cpu(out.size_le); @@ -72,6 +79,11 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) qemu_co_queue_init(&pdu->complete); pdu_submit(pdu); } + + return; + +out_free_pdu: + pdu_free(pdu); } static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,
A broken guest may send a request with only non-empty out buffers or only non-empty in buffers, virtqueue_pop() will then return a VirtQueueElement with out_num == 0 or in_num == 0 respectively. All 9P requests are expected to start with the following 7-byte header: uint32_t size_le; uint8_t id; uint16_t tag_le; If iov_to_buf() fails to return these 7 bytes, then something is wrong in the guest. In both cases, it is wrong to crash QEMU, since the root cause lies in the guest. Let's switch the device to the broken state instead. Signed-off-by: Greg Kurz <groug@kaod.org> --- v2: - added out_free_pdu: label for errors or when virtqueue is empty --- hw/9pfs/virtio-9p-device.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)