| Message ID | 147446364067.4880.17009801693705082626.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
On Wed, 21 Sep 2016 15:14:00 +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> > --- > hw/9pfs/virtio-9p-device.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > index 009b43f6d045..0f09bef13392 100644 > --- a/hw/9pfs/virtio-9p-device.c > +++ b/hw/9pfs/virtio-9p-device.c > @@ -56,13 +56,23 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) > break; > } > > - 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"); > + pdu_free(pdu); > + return; Make that 'break;' to be more consistent with the code right above? > + } > 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) { I always find 'sizeof foo' instead of 'sizeof(foo)' a bit jarring... > + virtio_error(vdev, "The guest sent a malformed VirtFS request: " > + "header size is %zd, should be 7", len); > + pdu_free(pdu); > + return; > + } > > pdu->size = le32_to_cpu(out.size_le); > > Looks good to me.
On Wed, 21 Sep 2016 16:16:59 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Wed, 21 Sep 2016 15:14:00 +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> > > --- > > hw/9pfs/virtio-9p-device.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > > index 009b43f6d045..0f09bef13392 100644 > > --- a/hw/9pfs/virtio-9p-device.c > > +++ b/hw/9pfs/virtio-9p-device.c > > @@ -56,13 +56,23 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) > > break; > > } > > > > - 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"); > > + pdu_free(pdu); > > + return; > > Make that 'break;' to be more consistent with the code right above? > The code right above isn't an error path, unlike here. Maybe I should even add an out_err: label to make it explicit. > > + } > > 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) { > > I always find 'sizeof foo' instead of 'sizeof(foo)' a bit jarring... > > > + virtio_error(vdev, "The guest sent a malformed VirtFS request: " > > + "header size is %zd, should be 7", len); > > + pdu_free(pdu); > > + return; And same here. > > + } > > > > pdu->size = le32_to_cpu(out.size_le); > > > > > > Looks good to me. >
On Wed, 21 Sep 2016 16:16:59 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Wed, 21 Sep 2016 15:14:00 +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> > > --- > > hw/9pfs/virtio-9p-device.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > > index 009b43f6d045..0f09bef13392 100644 > > --- a/hw/9pfs/virtio-9p-device.c > > +++ b/hw/9pfs/virtio-9p-device.c > > @@ -56,13 +56,23 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) > > break; > > } > > > > - 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"); > > + pdu_free(pdu); > > + return; > > Make that 'break;' to be more consistent with the code right above? > > > + } > > 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) { > > I always find 'sizeof foo' instead of 'sizeof(foo)' a bit jarring... > Oops... hit the send button to quickly :) I agree. I will fix this in a preparatory patch. > > + virtio_error(vdev, "The guest sent a malformed VirtFS request: " > > + "header size is %zd, should be 7", len); > > + pdu_free(pdu); > > + return; > > + } > > > > pdu->size = le32_to_cpu(out.size_le); > > > > > > Looks good to me. >
On Wed, 21 Sep 2016 16:38:26 +0200 Greg Kurz <groug@kaod.org> wrote: > On Wed, 21 Sep 2016 16:16:59 +0200 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Wed, 21 Sep 2016 15:14:00 +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> > > > --- > > > hw/9pfs/virtio-9p-device.c | 14 ++++++++++++-- > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > > > index 009b43f6d045..0f09bef13392 100644 > > > --- a/hw/9pfs/virtio-9p-device.c > > > +++ b/hw/9pfs/virtio-9p-device.c > > > @@ -56,13 +56,23 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) > > > break; > > > } > > > > > > - 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"); > > > + pdu_free(pdu); > > > + return; > > > > Make that 'break;' to be more consistent with the code right above? > > > > The code right above isn't an error path, unlike here. Maybe I should > even add an out_err: label to make it explicit. I think our tastes differ a bit. But it's your code :)
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c index 009b43f6d045..0f09bef13392 100644 --- a/hw/9pfs/virtio-9p-device.c +++ b/hw/9pfs/virtio-9p-device.c @@ -56,13 +56,23 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq) break; } - 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"); + pdu_free(pdu); + return; + } 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); + pdu_free(pdu); + return; + } pdu->size = le32_to_cpu(out.size_le);
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> --- hw/9pfs/virtio-9p-device.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)