Re: [Qemu-devel] [PATCH 1/7] virtio-9p: handle handle_9p_output() error

[Cornelia Huck]

On Wed, 21 Sep 2016 15:14:00 +0200
Greg Kurz <address@hidden> 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 <address@hidden>
> ---
>  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.