Re: [Qemu-devel] [PATCH v2 3/9] virtio-9p: handle handle_9p_output() error
On Fri, 23 Sep 2016 13:54:17 +0100
Stefan Hajnoczi 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
> > ---
> > 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
pgpQ8J0wC46MK.pgp
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 3/9] virtio-9p: handle handle_9p_output() error
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
> ---
> 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
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2 3/9] virtio-9p: handle handle_9p_output() error
On Wed, 21 Sep 2016 18:57:12 +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 > --- > 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
