Re: [Qemu-devel] [PATCH 1/7] virtio-9p: handle handle_9p_output() error
On Wed, 21 Sep 2016 16:38:26 +0200
Greg Kurz wrote:
> On Wed, 21 Sep 2016 16:16:59 +0200
> Cornelia Huck wrote:
>
> > On Wed, 21 Sep 2016 15:14:00 +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
> > > ---
> > > 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 :)
Re: [Qemu-devel] [PATCH 1/7] virtio-9p: handle handle_9p_output() error
On Wed, 21 Sep 2016 16:16:59 +0200
Cornelia Huck wrote:
> On Wed, 21 Sep 2016 15:14:00 +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
> > ---
> > 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.
>
Re: [Qemu-devel] [PATCH 1/7] virtio-9p: handle handle_9p_output() error
On Wed, 21 Sep 2016 16:16:59 +0200
Cornelia Huck wrote:
> On Wed, 21 Sep 2016 15:14:00 +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
> > ---
> > 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.
>
Re: [Qemu-devel] [PATCH 1/7] virtio-9p: handle handle_9p_output() error
On Wed, 21 Sep 2016 15:14:00 +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
> ---
> 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.
